Last Comment Bug 340956 - PR_Write in append mode fails for big files (4GB+/32bit border)
: PR_Write in append mode fails for big files (4GB+/32bit border)
: fixed1.8.0.5, fixed1.8.1
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: other
: x86 Windows XP
: -- normal (vote)
: 4.6.3
Assigned To: Wan-Teh Chang
: Wan-Teh Chang
: 299598 (view as bug list)
Depends on:
Blocks: 331647 334496
  Show dependency treegraph
Reported: 2006-06-09 06:36 PDT by Nils Maier [:nmaier]
Modified: 2007-04-23 03:39 PDT (History)
6 users (show)
dveditz: blocking1.8.0.5-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

v1 (906 bytes, patch)
2006-06-09 16:48 PDT, Nils Maier [:nmaier]
VYV03354: review-
Details | Diff | Splinter Review
using PROffset64 (1.05 KB, patch)
2006-06-09 18:28 PDT, Masatoshi Kimura [:emk]
wtc: review+
wtc: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
resolved review comment (1.02 KB, patch)
2006-06-10 06:47 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
VYV03354: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Nils Maier [:nmaier] 2006-06-09 06:36:29 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv: Gecko/20060508 Firefox/
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv: Gecko/20060508 Firefox/

PR_Write "emulates" the O_APPEND flag by seeking to EOF upon each write when in append mode.

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 2006-06-09 09:47:47 PDT
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:
Comment 2 Nils Maier [:nmaier] 2006-06-09 16:48:32 PDT
Created attachment 225092 [details] [diff] [review]

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] 2006-06-09 18:24:01 PDT
This causes bug 331647.
With this patch, I could download Windows Vista beta 2 x64 successfully :)
Comment 4 Masatoshi Kimura [:emk] 2006-06-09 18:26:52 PDT
Comment on attachment 225092 [details] [diff] [review]

> -        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] 2006-06-09 18:28:09 PDT
Created attachment 225105 [details] [diff] [review]
using PROffset64
Comment 6 Nils Maier [:nmaier] 2006-06-09 18:50:17 PDT
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
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] 2006-06-09 19:04:54 PDT
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] 2006-06-09 19:49:08 PDT
Per bug 308587, NSPR also doesn't support platforms without native 64-bit integer type anymore.
Comment 9 Wan-Teh Chang 2006-06-10 06:20:08 PDT
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] 2006-06-10 06:47:05 PDT
Created attachment 225122 [details] [diff] [review]
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) 2006-06-10 07:25:46 PDT
I think we should consider this for 1.8.0.x too.
Comment 12 Wan-Teh Chang 2006-06-10 20:07:35 PDT
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

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

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

Checking in prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision:; previous revision:
Comment 13 Nils Maier [:nmaier] 2006-06-10 20:55:44 PDT
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] 2006-06-12 11:21:28 PDT
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 2006-06-12 13:32:23 PDT
I checked in the patch on the MOZILLA_1_8_0_BRANCH

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

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.
Comment 16 Wayne Mery (:wsmwk, NI for questions) 2007-04-23 03:39:27 PDT
*** Bug 299598 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.