340956 - PR_Write in append mode fails for big files (4GB+/32bit border)

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Nils Maier [:nmaier]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

PR_Write "emulates" the O_APPEND flag by seeking to EOF upon each write when in append mode.
compare:
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/prfile.c#103

This is fine, but PR_Seek instead of PR_Seek64 is used so that this fails one we pass the 4GB "border".

I think this at least affects TB (no folders >4GB) as well as the downTHEMAll extension (found this hacking the latter).


Reproducible: Always

Steps to Reproduce:
1. Open an nsIFileOutputStream and write more than 4GB data. (Not at once)
2. See it throw once 4GB mark is reached.

Comment 1

[Wan-Teh Chang]

Nils: thank you for the bug report.  I believe the fix
is to change PR_Seek to PR_Seek64 as you pointed out.
Since you have the setup to test the fix, could you
submit a patch and test it?

Darin, it may be a good idea to review all the uses
of PR_Seek and see if any of them needs to be changed
to PR_Seek64:
http://lxr.mozilla.org/seamonkey/ident?i=PR_Seek

Comment 2

[Nils Maier [:nmaier]]

Attachment: v1

tested using a trunk build from a freshly pulled tree.
works with:
XP SP2 (Pro)
W2k SP4

PS: I lack everything, knowledge of the review process and obviously checkin privileges.

Comment 3

[Masatoshi Kimura [:emk]]

This causes bug 331647.
With this patch, I could download Windows Vista beta 2 x64 successfully :)

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 225092 [details] [diff] [review]
v1

> -        rv = PR_Seek(fd, 0, PR_SEEK_END );
> +        rv = PR_Seek64(fd, 0, PR_SEEK_END );
|rv| has only 32 bit width. What happens if PR_Seek64 returns 0xffffffff?

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: using PROffset64

Comment 6

[Nils Maier [:nmaier]]

err. turns out my first submitted piece of code is actually a piece of...
Was too easy to get it right.

Thanks for the correction.

Looking even more at this reveals this:
PROffset64 -> PRInt64
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#397
PRInt64 might be a struct if there is no __int64 and long long.
Remarks say LL_ macros should be used to ensure portable code.

I leave it to you experienced guys to figure this out. ;)

Comment 7

[Masatoshi Kimura [:emk]]

I heard LL_ macros are obsolete from Darin on bug 333703.
Does NSPR still support no __int64 platforms unlike Mozilla product?

Comment 8

[Masatoshi Kimura [:emk]]

Per bug 308587, NSPR also doesn't support platforms without native 64-bit integer type anymore.

Comment 9

[Wan-Teh Chang]

Comment on attachment 225105 [details] [diff] [review]
using PROffset64

Good catch for the PR_Seek64 return type issue
and good code cleanup.

Let's use a different variable name such as 'offset'
or 'off' for the return value of the PR_Seek64 call,
or just say:

+        if (PR_Seek64(fd, 0, PR_SEEK_END) == -1) {
+            return -1;
         }

On the LL_ macros: new code doesn't need to use the
LL_ macros because all the compilers that we use now
support a 64-bit integer type, which may be 'long long',
'long', or '__int64'.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: resolved review comment

Thanks for your review. Carrying over r+branch181.
Could you land the patch, please?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

I think we should consider this for 1.8.0.x too.

Comment 12

[Wan-Teh Chang]

Nils, I'm curious how you tracked down this bug.  This
bug takes a 4GB download to reproduce.  It's not obvious
how you catch it in action at the right moment.

We need to test this fix on Windows and OS/2.  This fix
does not affect BeOS and Unix (including Linux and Mac
OS X).

I checked in the bug on the NSPR trunk (NSPR 4.7 Beta),
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.9 alpha),
NSPR_4_6_BRANCH (NSPR 4.6.3 Beta), and MOZILLA_1_8_BRANCH
(Mozilla 1.8.1 beta1).

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.45; previous revision: 3.44
done

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.36.2.9; previous revision: 3.36.2.8
done

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.42.2.2; previous revision: 3.42.2.1
done

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.36.2.6.16.2; previous revision: 3.36.2.6.16.1
done

Comment 13

[Nils Maier [:nmaier]]

Download of WinVista Beta2 ISO was reported to always fail with downTHEMall.
First I replicated that problem.
Downloading a 4.3GB file from my local apache.
Using sysinternal's filemon. Which showed a failed seek.
There is no seek() in dTa, Windows won't seek, so it must be Firefox.

Then I simply went to lxr and traced the callstack :p
nsFileOutputStream uses PR_Write.
PR_Write is fd->methods->write.
which is a member of PRIOMethods declared in prio.h and referenced by prio.c
which then gives FileWrite, i.e. the buggy function.
Saw the PR_Seek (and the FileSeek and FileSeek64 near FileWrite).

Was hidden damn well. Took some time to find it.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 225122 [details] [diff] [review]
resolved review comment

Not blocking, but approved for 1.8.0 branch, a=dveditz for drivers

Comment 15

[Wan-Teh Chang]

I checked in the patch on the MOZILLA_1_8_0_BRANCH
(Mozilla 1.8.0.5).

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.36.2.6.24.1; previous revision: 3.36.2.6
done

Nils, it seems that the "breakthrough" in your debugging
was that sysinternal's filemon showed a failed seek.
That pointed you in the right direction.