266215 - _MD_writev is broken on some 64-bit platforms due to PRIOVec mismatch with struct iovec

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Julien Pierre]

On Solaris, in 64-bit mode, both Sparc and AMD64, the iovec structure contains a
length of size_t, which is 64 bit.
But PRIOVec only contains an int iovec , which is 32 bits since Solaris uses LP64 .

In unix.c, _MD_writev does :

while ((rv = writev(osfd, (const struct iovec*)iov, iov_size)) == -1) {

where iov is a PRIOVec * .

The result is that the incorrect length will be read. The upper or lower 32 bits
of the length in the vector will be incorrectly read, because there is no memory
allocated for them inside the PRIOVec structure.

Comment 1

[Julien Pierre]

Actually, due to structure packing, sizeof(PRIOVec) and sizeof(struct iovec) is
the same, 16 bytes, on 64-bit Sparc and AMD64 platforms . But when the OS writev
executes, it gets the wrong value, since only the first 32 bits are filled.

Comment 2

[Wan-Teh Chang]

Julien, the _MD_writev code in unix.c is broken,
but it is not being used on Solaris.  On Solaris,
we use the pt_Writev code in ptio.c, which doesn't
have this problem.

We should modify _MD_writev to do the same thing
as pt_Writev.

If you can verify that _MD_writev is dead code
for Solaris/Unix, please lower the priority of
this bug and target it at NSPR 4.6.

We should also do something about this comment in
unix.c:

/*
 * There is an assertion in this code that NSPR's definition of PRIOVec
 * is bit compatible with UNIX' definition of a struct iovec. This is
 * applicable to the 'writev()' operations where the types are casually
 * cast to avoid warnings.
 */

Comment 3

[Julien Pierre]

Wan-Teh,

You are right, the code I looked at was the wrong one. _MD_writev isn't used on
Solaris, so we are OK.

Which 64-bit platforms might the _MD_writev be used with ? If there aren't any,
perhaps this bug should just be closed invalid.

The problem that was reported to me was actually in an application's NSPR layer,
which did a cast between iovec and PRIOVec, and broke on Solaris Sparcv9/AMD64.
This bug was more pervasive on Sparc actually due to endianness.

Comment 4

[Wan-Teh Chang]

_MD_writev is used on non-Unix platforms and
Unix platforms that don't have pthreads.

This is a valid bug, it's just that it has a
low priority because it hasn't broken any
known 64-bit platform yet.

The fix is not hard.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch

Comment 6

[Julien Pierre]

Wan-Teh,

It is actually possible to test if the iovec and PRIOVec structures are
compatible, which will be the case on most platforms . I believe for example it
is always true on OS/2 .

I tried to do this check at compile time using the C pre-processor, but did not
succeed. gcc wouldn't allow me to use sizeof or offsetof with #if statements .

However, it is certainly possible to do this check once at runtime during NSPR
initialization, using the sizeof() and offsetof() macros, against the iovec and
PRIOVec fields. Once this is determined, the conversion could be avoided altogether.

Comment 7

[Wan-Teh Chang]

Julien, we can do that in unix.c, _PR_UnixInit().
For OS/2 and BeOS we can omit that runtime check
because we've already (manually) verified that to
be true for these two platforms.

Comment 8

[Julien Pierre]

Attachment: add runtime check for matching PRIOVec and struct iovec

Comment 9

[Wan-Teh Chang]

Comment on attachment 243556 [details] [diff] [review]
add runtime check for matching PRIOVec and struct iovec

Julien, you attached the wrong patch.

Comment 10

[Julien Pierre]

Attachment: Correct patch (except changes to unix.c)

Comment 11

[Wan-Teh Chang]

Comment on attachment 243711 [details] [diff] [review]
Correct patch (except changes to unix.c)

r=wtc.  Do not check in the changes to unix.c.

I suggest that you also change 'iov' to 'osiov' in

     if (!fd->secret->nonblocking) {
         for (index=0; index<iov_size; index++) {
             amount += iov[index].iov_len;
         }
     }

Regarding unix.c: this file is now only used on the few
Unix platforms that still don't have pthreads.  So it's
not worth our time to optimize (using the
PRIOVec_is_iovec boolean variable) for those few platforms.
I'd also need to do a special build with the
-enable-nspr-threads option to test the changes to unix.c.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Julien, what is the status of this bug?

Comment 13

[Julien Pierre]

Checking in src/md/beos/bnet.c;
/cvsroot/mozilla/nsprpub/pr/src/md/beos/bnet.c,v  <--  bnet.c
new revision: 3.16; previous revision: 3.15
done
Checking in src/md/os2/os2sock.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2sock.c,v  <--  os2sock.c
new revision: 3.19; previous revision: 3.18
done