The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 4.6.3

Status

NSPR
NSPR
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: nmaier, Assigned: Wan-Teh Chang)

Tracking

({fixed1.8.0.5, fixed1.8.1})

other
4.6.3
x86
Windows XP
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph
Bug Flags:
blocking1.8.0.5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

11 years ago
Created attachment 225092 [details] [diff] [review]
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.
This causes bug 331647.
With this patch, I could download Windows Vista beta 2 x64 successfully :)
Blocks: 331647
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?
Attachment #225092 - Flags: review-
Created attachment 225105 [details] [diff] [review]
using PROffset64
Attachment #225092 - Attachment is obsolete: true
Attachment #225105 - Flags: review?(wtchang)
Attachment #225105 - Flags: approval-branch-1.8.1?(wtchang)
(Reporter)

Comment 6

11 years ago
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. ;)
I heard LL_ macros are obsolete from Darin on bug 333703.
Does NSPR still support no __int64 platforms unlike Mozilla product?
Per bug 308587, NSPR also doesn't support platforms without native 64-bit integer type anymore.
(Assignee)

Comment 9

11 years ago
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'.
Attachment #225105 - Flags: review?(wtchang)
Attachment #225105 - Flags: review+
Attachment #225105 - Flags: approval-branch-1.8.1?(wtchang)
Attachment #225105 - Flags: approval-branch-1.8.1+
Created attachment 225122 [details] [diff] [review]
resolved review comment

Thanks for your review. Carrying over r+branch181.
Could you land the patch, please?
Attachment #225105 - Attachment is obsolete: true
Attachment #225122 - Flags: review+
Attachment #225122 - Flags: approval-branch-1.8.1+
Blocks: 334496
I think we should consider this for 1.8.0.x too.
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
(Assignee)

Comment 12

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Target Milestone: --- → 4.6.3
(Reporter)

Comment 13

11 years ago
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.
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5-
Comment on attachment 225122 [details] [diff] [review]
resolved review comment

Not blocking, but approved for 1.8.0 branch, a=dveditz for drivers
Attachment #225122 - Flags: approval1.8.0.5+
(Assignee)

Comment 15

11 years ago
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.
Keywords: fixed1.8.0.5
Duplicate of this bug: 299598
You need to log in before you can comment on or make changes to this bug.