Closed Bug 215450 Opened 21 years ago Closed 12 years ago

uploading files that are larger the 2GB fails

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: boris.tabenkin, Assigned: mayhemer)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 39 obsolete files)

102.25 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
981 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review
7.01 KB, patch
sicking
: review+
joe
: review+
Details | Diff | Splinter Review
17.85 KB, patch
neil
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529

When trying to upload a file that is larger then 2GB the browser does nothing. 

Reproducible: Didn't try

Steps to Reproduce:
1. build a html form thjat has a <input type="file"> tag
2. submit the form
3.
Summary: uplaoding files that are larger the 2GB fails → uploading files that are larger the 2GB fails

*** This bug has been marked as a duplicate of 215091 ***
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---

*** This bug has been marked as a duplicate of 184452 ***
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → DUPLICATE
Best expressed as a dependency, not a duplicate. Someone will have to test each
entrypoint that is affected by the possible fix of 184452.
Status: RESOLVED → UNCONFIRMED
Depends on: 184452
Resolution: DUPLICATE → ---

*** This bug has been marked as a duplicate of 184452 ***
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → DUPLICATE
please let this bug open and read comment 4
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Does the submit form use http: URL? If so, please move this to HTTP.
Status: UNCONFIRMED → NEW
Component: File Handling → Networking
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Assignee: law → darin
QA Contact: chrispetersen → benc
I wonder if this is still a problem in recent builds given all of biesi's work
on supporting large files.
I didn't modify the frozen nsIUploadChannel, neither the frozen nsIInputStream,
which means there is no way for the HTTP code to know it has to upload more than
2 GB :-/ so, I would be surprised if this is fixed now
true, true!  if we supported uploads without an explicit content-length header,
then we'd be able to support >2GB uploads with this API, but otherwise you are
correct... we need a new API.  maybe nsIUploadChannel2 is all that is needed, or
perhaps this could be another for that properties API I keep talking about ;-)
Assignee: darin → nobody
QA Contact: benc → networking
Any update on this issue? 
With the current usage scenario, 2 GB limit is really small. Google Chrome don't have a limitation of 2GB.
Attached patch WIP v0.5 (obsolete) — Splinter Review
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → m_kato
Attachment #420061 - Attachment is obsolete: true
Attachment #420283 - Flags: review?(cbiesinger)
Comment on attachment 420283 [details] [diff] [review]
patch v1

Thanks for the patch! A few comments:

+++ b/netwerk/base/src/nsBufferedStreams.h
+                NS_ASSERTION(avail < PR_UINT32_MAX,
+                             "Source() should have nsIInputStream2 to support 64-bit stream");

This assertion is incorrect. This assertion fires when the stream has exactly 4 GB of data available. It doesn't seem like that should be disallowed.

+++ b/netwerk/base/src/nsFileStreams.cpp
+    // PR_Available with files over 4GB returns an error, so we have to
+    // use the 64-bit version.  Available64() uses 64-bit version.

I don't think that comment makes sense anymore. Just remove it?

+++ b/netwerk/base/src/nsMIMEInputStream.cpp
+                    NS_ASSERTION(cl32 < PR_UINT32_MAX,
+                                 "mData should have nsIInputStream2 to support 64-bit stream");

as above

+    nsCOMPtr<nsIInputStream2> stream2 = do_QueryInterface(mStream);

mStream is an nsIMultiplexInputStream, which inherits nsIInputStream2. Consequently, this entire method can just be "return mStream->Available64(_retval);"

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+                    // If return value is PR_UINT32_MAX, stream has over 4GB.

That conclusion is not valid. The value can also mean that it has exactly 4 GB.

I think it's OK to leave the code as you've written it, but please change the comment to say "the stream probably has more than 4 GB".

+            // If return value is PR_UINT32_MAX, stream has over 4GB.

same

+++ b/netwerk/protocol/http/src/nsHttpChannel.h
-#include "nsIInputStream.h"
+#include "nsIInputStream2.h"

Why this change? nsIInputStream2 is not used in the header, only nsIInputStream is.

+++ b/xpcom/io/nsIInputStream2.idl
+    long long available64();

Why is this signed? I think for consistency with nsIInputStream, and because negative numbers make no sense, it would be better to make this unsigned.

+++ b/xpcom/io/nsMultiplexInputStream.cpp
+                NS_ASSERTION(avail32 < PR_UINT32_MAX, "mStreams[i] should have nsIInputStream2 to support 64-bit stream");

again, I'd prefer you to remove this assertion
Attachment #420283 - Flags: review?(cbiesinger) → review-
I'm not entirely happy with this patch, since a 64-bit available shouldn't be necessary, but it seems like it may be the only way to get the total content length of the post data.
Thank you for reviewing!

(In reply to comment #15)

> +++ b/xpcom/io/nsIInputStream2.idl
> +    long long available64();
> 
> Why is this signed? I think for consistency with nsIInputStream, and because
> negative numbers make no sense, it would be better to make this unsigned.

Because 64-bit interface of NSPR uses signed long long.  I will modify this interface to unsigned.
Attached patch patch v2 (obsolete) — Splinter Review
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #424724 - Attachment is obsolete: true
Attachment #424728 - Flags: review?(cbiesinger)
Makoto, now that we have gone through the Great XPCOM Interface Unfreezing you can modify nsIInputStream directly.
Could we promote this bug up a bit somehow? This is 7 years old, relatively easy to fix bug (or so it looks from the patches posted) that will give Firefox another advantage in browser space (currently only Chrome is able to upload big files).
(In reply to comment #21)
> Could we promote this bug up a bit somehow? This is 7 years old, relatively
> easy to fix bug (or so it looks from the patches posted) that will give Firefox
> another advantage in browser space (currently only Chrome is able to upload big
> files).

This is needed API change (Now API is freezed).  After Firefox 4 / Gecko 2.0 is released, I will raise the priority.
I am glad you are on it!

But why not have a stop-gap solution in Firefox 4 and then have another bug to fix it properly in the new API? (Like actually complete the upload even if we don't send the size header or show an incorrect progress indicator for files over 2 or 4 Gb)
2Comment21
Now,IE9 (x64 at least) is able to upload >2Gb too.
@Igor: 
x86 also can
Attached patch fix v3 (obsolete) — Splinter Review
I should separate this to multiple...  Now testing.
Attachment #424728 - Attachment is obsolete: true
Attachment #424728 - Flags: review?(cbiesinger)
For what it's worth, we no longer have any frozen interfaces, so we should change all of the relevant ones to use 64bit instead of 32bit.
Josh, could we get someone on the network team to pick this up?
Maybe Jason can take a look?
Assignee: m_kato → jduell.mcbugs
Comment on attachment 539442 [details] [diff] [review]
fix v3

Review of attachment 539442 [details] [diff] [review]:
-----------------------------------------------------------------

One thing we'll need to add to this patch is e10s support--I'm guessing there's some work to do there.  See PHttpChannel.ipdl and the HttpChannel{Child,Parent} classes (and the FTP versions too).  There may be 32 bit data we're sending between parent/child that needs to be 64 bit.

Makoto--are you still interested in working on this bug?  I'm happy to let you do the fixes and I can advise and review patches.
I think Steve was looking at this bug earlier today.
Yup, been looking through this at Josh's request.

Quick clarification on Jonas's comment #28:
The diff currently changes the interface and classes implementing it, but there are several functions calling available() that don't support 64bits.  I.e. they call other functions which only support 32 bits.  Makato's diff deals with these by checking for > 32 bits and casting or returning (e.g. nsFrameScriptExecutor::LoadFrameScriptInternal())

Assuming everyone is ok with this, and a rewrite of those other functions isn't required?

Also, ref comment #15, we can change some of the assertions to (size <= PR_UINT32_MAX), right?
> there are several functions calling available() that don't support 64bits.
> I.e. they call other functions which only support 32 bits.  Makato's diff
> deals with these by checking for > 32 bits and casting or returning (e.g.
> nsFrameScriptExecutor::LoadFrameScriptInternal())

That's *probably* fine. But if you could look at all these code sites and try to get a sense of what the repercussions for failing/skipping on > PR_UINT32_MAX is, that would be great.  

In other words, time for your first review! :)

Turns out I'm wrong about needing e10s work--doesn't look like we need to do anything for it.

> Also, ref comment #15, we can change some of the assertions to 
> (size <= PR_UINT32_MAX), right?

Seems like these have been taken out of the latest version?  It's OK to have them, as long as they make sense (i.e they're not testing against an unsigned 32-bit 'size', in which case they'd always be true).

Once you've fixed any bitrot in the patch, looked over it, and identified any places where things look problematic (or you just can't figure them out yourself), let me know, and I can do another +r.
Comment on attachment 539442 [details] [diff] [review]
fix v3

Since it's your first review, let's call it "feedback"
Attachment #539442 - Flags: review?
Attachment #539442 - Flags: feedback?(sjhworkman)
Assignee: jduell.mcbugs → sjhworkman
Attached patch fix v4 (obsolete) — Splinter Review
Attachment #420283 - Attachment is obsolete: true
Attachment #539442 - Attachment is obsolete: true
Attachment #539442 - Flags: review?
Attachment #539442 - Flags: feedback?(sjhworkman)
Attachment #554809 - Flags: feedback?(sjhworkman)
Comment on attachment 554809 [details] [diff] [review]
fix v4

Review of attachment 554809 [details] [diff] [review]:
-----------------------------------------------------------------

Please note:  I'm new to this code, so some of my feedback comments may need their own feedback :)

General Comments:

-- nsGIOInputStream::Available(32) needs to be updated to 64 bits as well.

-- I suggest that ::OnDataAvailable() should also be upgraded from 32 bits to 64 bits (in a future bug, not this one).

::: content/base/src/nsFrameMessageManager.cpp
@@ +768,5 @@
>    channel->Open(getter_AddRefs(input));
>    nsString dataString;
> +  PRUint64 avail = 0;
> +  if (input && NS_SUCCEEDED(input->Available(&avail)) && avail &&
> +      avail < PR_UINT32_MAX) {

Why '<' and not '<='?

::: content/base/src/nsSyncLoadService.cpp
@@ +386,5 @@
>      rv = aListener->OnStartRequest(aChannel, nsnull);
>      if (NS_SUCCEEDED(rv)) {
>          PRUint32 sourceOffset = 0;
>          while (1) {
> +            PRUint64 readCount = 0;

In subsequent call to aListener->OnDataAvailable, can you add an explicit PRUint32 cast to readCount.

Also, if (readCount > PR_UINT32_MAX) can you set it to PR_UINT32MAX?  This way, OnDataAvailable will only be told about 4GB maximum at a time.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +329,4 @@
>    rv = stream->Available(&count);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    return Base64EncodeInputStream(stream, aDataURL, count, aDataURL.Length());

Base64EncodeInputStream only takes a PRUint32 for count.  Can you do an NS_ENSURE_TRUE(count <= PR_UINT32_MAX,...)?  And explicitly cast count as PRUint32.

@@ +369,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  // no support 4GB or over
> +  NS_ENSURE_TRUE(imgSize64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Same as previous: why not '<='?

::: content/media/nsMediaStream.cpp
@@ +1057,3 @@
>    rv = mInput->Available(&size);
>    if (NS_SUCCEEDED(rv)) {
>      mSize = size;

'mSize' is a PRInt64 - can you add a check for (size <= PR_INT64_MAX) and do an explicit cast?

::: content/xul/document/src/nsXULPrototypeCache.cpp
@@ +705,5 @@
> +        if (NS_SUCCEEDED(rv)) {
> +            PRUint64 len64;
> +            rv = inputStream->Available(&len64);
> +            len = PRUint32(len64);
> +            if (NS_SUCCEEDED(rv) && len64 >= PR_UINT32_MAX) {

Last time I'll mention this ;) why do you not allow PR_UINT32_MAX?

::: dom/src/json/nsJSON.cpp
@@ +531,5 @@
>    nsresult status;
>    jsonChannel->GetStatus(&status);
>    PRUint32 offset = 0;
>    while (NS_SUCCEEDED(status)) {
> +    PRUint64 available;

Subsequent call to OnDataAvailable uses a PRUint32.  Can you do a MAX check and set it to PR_UINT32_MAX if it's more than that.  Since this one is in a while loop, it should be ok to just get PR_UINT32_MAX for every loop.

Also, please do an explicit cast in the function call for OnDataAvailable to PRUint32.

@@ +672,5 @@
>  nsJSONListener::OnDataAvailable(nsIRequest *aRequest, nsISupports *aContext,
>                                  nsIInputStream *aStream,
>                                  PRUint32 aOffset, PRUint32 aLength)
>  {
> +  PRUint64 contentLength;

Looks good.

::: gfx/thebes/gfxASurface.cpp
@@ +784,5 @@
>    CallQueryInterface(encoder.get(), getter_AddRefs(imgStream));
>    if (!imgStream)
>      return;
>  
> +  PRUint64 bufSize;

Subsequent call to imgStream->Read is looking for a PRUint32 for 'aCount', but is passed (bufSize-imgSize), which could technically be more than PR_UINT32_MAX.
Can you add a check for NS_ENSURE_TRUE((bufSize-imgSize) < PR_UINT32_MAX, ...) please?

::: layout/base/nsPresShell.cpp
@@ +8600,5 @@
>    NS_ENSURE_TRUE(file, NS_ERROR_FAILURE);
>    rv = file->InitWithPath(name);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  PRUint64 length;

Needs to have a check for PR_UINT32_MAX.  Subsequent call to NS_NewBufferedOutputStream only accepts PRUint32 for length.

::: modules/libjar/nsJARChannel.cpp
@@ +164,5 @@
>          return rv;
>      }
>  
>      // ask the JarStream for the content length
> +    rv = mJarStream->Available((PRUint64 *) &mContentLength);

Better to use a new PRUint64 to get the available length, check for PR_INT32_MAX (mContentLength is 32 bits signed), and then set mContentLength if (newVar <= PR_INT32_MAX).

::: modules/libpr0n/src/imgTools.cpp
@@ +105,5 @@
>        inStream = bufStream;
>    }
>  
>    // Figure out how much data we've been passed
> +  PRUint64 length;

inStream->ReadSegments takes a PRUint32.  Please check for MAX and return as appropriate.  Add explicit casting to the function call.

::: netwerk/base/src/nsInputStreamChannel.cpp
@@ +50,5 @@
>    // the stream can tell us.  If the stream is a pipe, then this will not work.
>  
>    PRInt64 len = ContentLength64();
>    if (len < 0) {
> +    PRUint64 avail;

SetContentLength takes a PRInt64.  There needs to be some checking here to make sure PR_INT64_MAX isn't passed.  NS_ENSURE_TRUE(avail <= PR_INT64_MAX, ..) should be fine.

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +137,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
>    PeekData data(callback, closure);
> +  PRUint32 dummy32 = dummy;

Need to check for PR_UINT32_MAX here when setting dummy32.

@@ +536,5 @@
>                          mStatus = NS_ERROR_UNEXPECTED;
>                      }
>                  }
>                  else
> +                    mStreamOffset += avail32; // assume ODA behaved well

This all looks good :)

::: netwerk/base/src/nsMIMEInputStream.cpp
@@ +207,5 @@
>          if (mData) {
>              mData->Available(&cl);
>          }
>          mContentLength.AssignLiteral("Content-Length: ");
> +        mContentLength.AppendInt(cl);

AppendInt takes a PRInt32, so cl needs to be cast to PRInt32.  Also, it looks like this function should fail if (cl > PR_INT32_MAX).

::: netwerk/ipc/NeckoMessageUtils.h
@@ +214,3 @@
>  
>        aParam.mStream->Available(&bytes);
> +      if (bytes > 0 && bytes < PR_UINT32_MAX) {

I think this should fail more obviously if (bytes > PR_UINT32_MAX).  I suggest adding a separate if statement before this one...
if (bytes > PR_UINT32_MAX) {
  NS_WARNING("Cannot copy more than 4GB");
  return;
}
... and then continue with the next if statement.

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +429,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
>        mUploadLength = avail;

mUploadLength is a PRInt64.  Need to do NS_ENSURE_TRUE(avail <= PR_INT64_MAX, ...).

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +314,5 @@
>  }
>  
>  void
>  FTPChannelChild::DoOnDataAvailable(const nsCString& data,
> +                                   const PRUint64& offset,

In this function there is a call to OnDataAvailable which accepts a PRUint32 only.  I think there needs to be a cast/MAX check before OnDataAvailable is called.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +479,5 @@
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    PRUint64 streamLength = 0;
> +    if (NS_SUCCEEDED(aStream->Available(&streamLength))) {
> +      aContentLength = streamLength;

This assignment should be similar to the one in HttpBaseChannel::SetUploadStream()

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +343,5 @@
>  HttpChannelChild::OnTransportAndData(const nsresult& status,
>                                       const PRUint64 progress,
>                                       const PRUint64& progressMax,
>                                       const nsCString& data,
> +                                     const PRUint64& offset,

There is a call to OnDataAvailable in this function.  It takes a PRUint32 only.

::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp
@@ +126,5 @@
>  
>      nsAutoArrayPtr<char> buffer(new char[streamLen + 1]);
>      NS_ENSURE_TRUE(buffer, NS_ERROR_OUT_OF_MEMORY);
>  
>      rv = inStr->Read(buffer, streamLen, &read);

Read takes a PRUint32, so streamLen needs to be cast/checked for MAX.

::: netwerk/streamconv/test/Converters.cpp
@@ +96,5 @@
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return mListener->OnDataAvailable(request, ctxt, convertedStream, sourceOffset, PRUint32(len));

len also needs to be checked for (len > PR_UINT32_MAX).

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +100,5 @@
>  
>          char *buffer = (char*)nsMemory::Alloc(len + 1);
>          if (!buffer) return NS_ERROR_OUT_OF_MEMORY;
>  
>          rv = inStr->Read(buffer, len, &read);

len needs to be cast to PRUint32, and checked for (len > PR_UINT32_MAX)

@@ +141,3 @@
>      dataStream->Available(&avail);
>  
>      return aListener->OnDataAvailable(request, nsnull, dataStream, 0, avail);

avail needs to be cast to PRUint32, and checked for (len > PR_UINT32_MAX)

::: parser/xml/src/nsSAXXMLReader.cpp
@@ +556,2 @@
>      if (NS_SUCCEEDED(rv))
>        offset += available;

offset and available should be capped at PR_UINT32_MAX before calling OnDataAvailable.  Otherwise, offset may increment more that what was processed in OnDataAvailable.

if (offset > PR_UINT32_MAX) offset = PR_UINT32_MAX;
same for available.
Then cast to PRUint32 in the function call.

::: rdf/base/src/nsRDFXMLDataSource.cpp
@@ +565,5 @@
>  
>          if (avail == 0)
>              break; // eof
>  
>          rv = aConsumer->OnDataAvailable(channel, nsnull, bufStream, offset, avail);

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX
OnDataAvailable( ... (PRUint32)avail);

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2778,5 @@
>  
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
> +  PRUint64 len = aLen == PR_UINT32_MAX ? n : aLen;

Please add parentheses for clarity.

PRUint64 len = (aLen == PR_UINT32_MAX) ? n : aLen;

::: startupcache/StartupCacheUtils.cpp
@@ +120,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(avail64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Why '<' and not '<='?

::: toolkit/components/places/nsFaviconService.cpp
@@ +550,5 @@
>      return NS_ERROR_FAILURE;
>  
>    // read all the decoded data
>    PRUint8* buffer = static_cast<PRUint8*>
>                                 (nsMemory::Alloc(sizeof(PRUint8) * available));

This is fine for the cast, but Read is called later and only take PRUint32.  I suggest you use NS_ENSURE_TRUE((available <= PR_UINT32_MAX)...).

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +2293,5 @@
>    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "SyncChannelStatus failed");
>  
>    while (NS_SUCCEEDED(rv))
>    {
> +    PRUint64 available;

OnDataAvailable is called later, so available needs to be capped at PR_UINT32_MAX before calling.

::: xpcom/components/nsComponentManager.cpp
@@ +573,5 @@
>  
>          if (avail > flen)
>              return;
>  
> +        if (NS_FAILED(is->Read(whole + totalRead, (PRUint32)avail, &read)))

Rather than just casting avail, it is better to cap it to PR_UINT32_MAX.

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX;
...Read(...PRUint32(avail) ...);

::: xpcom/io/Base64.cpp
@@ +196,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // no support 4GB or over
> +    NS_ENSURE_TRUE(count < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Why '<' and not '<='?

::: xpcom/io/nsPipe3.cpp
@@ +743,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

Did you consider changing mAvailable to a PRUint64?

::: xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp
@@ +184,3 @@
>          PRUint32 read;
>  
>          if (NS_FAILED(stream->Available(&avail)))

Even though you already checked for (flen > PR_UINT32_MAX), I think it's a good idea to cap avail here before you call Read().  Just some defensive programming.

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX;
...Read( ... (PRUint32)avail ..)
>::: netwerk/ipc/NeckoMessageUtils.h
>@@ +214,3 @@
>>  
>>        aParam.mStream->Available(&bytes);
>> +      if (bytes > 0 && bytes < PR_UINT32_MAX) {
>
>I think this should fail more obviously if (bytes > PR_UINT32_MAX).  I suggest >adding a separate if statement before this one...
>if (bytes > PR_UINT32_MAX) {
>  NS_WARNING("Cannot copy more than 4GB");
>  return;
>}
>... and then continue with the next if statement.

This function must be infallible, so you should use NS_ABORT_IF_FALSE instead.
Comment on attachment 554809 [details] [diff] [review]
fix v4

Stealing review here on Jason's request.
Attachment #554809 - Flags: feedback?(sjhworkman) → feedback?(honzab.moz)
Assignee: sjhworkman → m_kato
Attachment #554809 - Flags: feedback?(honzab.moz) → review?(honzab.moz)
Attached patch fix v5 (obsolete) — Splinter Review
Attachment #554809 - Attachment is obsolete: true
Attachment #563032 - Flags: review?(honzab.moz)
Attachment #554809 - Flags: review?(honzab.moz)
I started reviewing this.  I'm at about half, please don't modify the patch until I publish the comments.  Thanks.
Comment on attachment 563032 [details] [diff] [review]
fix v5

Review of attachment 563032 [details] [diff] [review]:
-----------------------------------------------------------------

r-

This is just the first part of the review, I still have to finish few other files and also check on functionality indirectly affecting this patch.  Publishing this just because I'm paranoid and afraid of loosing the comments.  In other words, it is not yet time to update the patch probably ;)

::: content/base/src/nsSyncLoadService.cpp
@@ +402,2 @@
>              rv = aListener->OnDataAvailable(aChannel, nsnull, aIn,
> +                                            NS_MIN(sourceOffset, PRUint64(PR_UINT32_MAX)),

Result should be explicitly cast to PRUint32.  That will be to long - please do:

PRUint32 sourceOffset32 = (PRUint32)NS_MIN(sourceOffset, PRUint64(PR_UINT32_MAX));

and use sourceOffset32 in ODA call.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2351,2 @@
>        postDataStream->Available(&uploadTotal);
>        mUploadTotal = uploadTotal;

You can just: postDataStream->Available(&mUploadTotal);

::: content/media/nsMediaStream.cpp
@@ +1057,3 @@
>    rv = mInput->Available(&size);
>    if (NS_SUCCEEDED(rv)) {
>      mSize = size;

mSize is signed int, you need to check for PR_INT64_MAX before you assign and fail with some NS_ERROR_FILE_TOO_BIG or OOM.

::: dom/src/json/nsJSON.cpp
@@ +536,1 @@
>      rv = aStream->Available(&available);

This has to be the same code as in nsSyncLoadService.cpp (truncate to max of PR_UINT32, cast to PRUint32, call ODA() and loop again).

@@ +677,2 @@
>    aStream->Available(&contentLength);
>    nsresult rv = NS_OK;

I think you can remove these 3 lines of code at all.

::: extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
@@ +468,4 @@
>  
>        // Update the content length attribute on the channel.  We do this
>        // synchronously without proxying.  This hack is not as bad as it looks!
> +      if (mBytesRemaining != PR_UINT64_MAX)

This should be if (mBytesRemaining <= PR_UINT32_MAX).

(Bug 233305 comment 3).

::: extensions/pref/autoconfig/src/nsReadConfig.cpp
@@ +314,5 @@
>  
>      PRUint32 fs, amt = 0;
> +    PRUint64 fs64;
> +    rv = inStr->Available(&fs64);
> +    NS_ENSURE_SUCCESS(rv, rv);

use if (NS_FAILED(rv)) return rv here to be consistent with the rest of the file.

@@ +316,5 @@
> +    PRUint64 fs64;
> +    rv = inStr->Available(&fs64);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // XXX no support over 4GB
> +    NS_ENSURE_TRUE(fs64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Out of scope of this bug: 64bit build should be able to allocate >4GB.  The check should be for ~PRSize(0), IMO.  But it doesn't belong here.

::: gfx/thebes/gfxASurface.cpp
@@ +793,5 @@
>  
>    // ...leave a little extra room so we can call read again and make sure we
>    // got everything. 16 bytes for better padding (maybe)
>    bufSize += 16;
> +  NS_ENSURE_TRUE(bufSize <= PR_UINT32_MAX, );

Just return, this is quit dirty.  And the check should be before the += 16 for PR_UINT32_MAX - 16.

@@ +806,5 @@
>    {
>      imgSize += numReadThisTime;
>      if (imgSize == bufSize) {
>        // need a bigger buffer, just double
>        bufSize *= 2;

Before this line we should check for for bufSize <= PR_UINT32_MAX / 2.

::: js/src/xpconnect/loader/mozJSComponentLoader.cpp
@@ +963,3 @@
>              NS_ENSURE_SUCCESS(rv, rv);
> +            // no support 4GB or over script stream
> +            NS_ENSURE_TRUE(len64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Condition *is* correct here.  Return FILE_TOO_BIG?

::: layout/base/nsPresShell.cpp
@@ +7833,5 @@
>    rv = file->InitWithPath(name);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  PRUint64 length64;
> +  rv = encoder->Available(&length);

length64 as an arg!  (there had to be a warning on the line you use length64, treat warnings as errors please).

@@ +7835,5 @@
>  
> +  PRUint64 length64;
> +  rv = encoder->Available(&length);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(length64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG?

::: modules/libjar/nsJARChannel.cpp
@@ +164,5 @@
>          return rv;
>      }
>  
>      // ask the JarStream for the content length
> +    rv = mJarStream->Available((PRUint64 *) &mContentLength);

No.  This should first read to PRUint64 local var, check for being < PR_INT32_MAX, if not, fallback (or just leave) mContentLength to -1.

::: modules/libpref/src/Preferences.cpp
@@ +819,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
> +  // XXX no support over 4GB
> +  NS_ENSURE_TRUE(fileSize64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +470,5 @@
>          avail = 0;
>      }
>      else if (NS_SUCCEEDED(rv) && avail) {
>          // figure out how much data to report (XXX detect overflow??)
> +        if (avail + mStreamOffset > mStreamLength)

Could (avail + mStreamOffset) ever overflow?

@@ +504,5 @@
>              PRUint32 odaOffset =
>                  mStreamOffset > PR_UINT32_MAX ?
>                  PR_UINT32_MAX : PRUint32(mStreamOffset);
>  
> +            PRUint32 odaAvail = PRUint32(NS_MIN(avail, PRUint64(PR_UINT32_MAX)));

Maybe use the same code as used for odaOffset to be consistent?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +477,5 @@
>      if (contentType.IsEmpty()) {
>        mUploadStreamHasHeaders = PR_TRUE;
>        mRequestHead.SetMethod(nsHttp::Post); // POST request
>      } else {
>        if (contentLength < 0) {

I think we will have to change this API, len == 0 means "do get length your self", instead of len == -1.

REVIEW TODO: think of this API change request a bit more

@@ +482,5 @@
>          // Not really kosher to assume Available == total length of
>          // stream, but apparently works for the streams we see here.
> +        PRUint64 contentLength64;
> +        nsresult rv = stream->Available(&contentLength64);
> +        if(NS_FAILED(rv) || contentLength64 > PR_INT32_MAX) {

This is one of the core places to fix this bug actually.

If contentLength arg is < 0, then why not accept CL of full 64 bits?

Only if call to Available() failed, return NS_ERROR_FAILURE.

Actually, contentLength64 should be whole |if (stream) { }| block scope var.  If contentLength arg is >= 0, then assign it to contentLength64 and use contentLength64 for the rest of the logic.

@@ +487,5 @@
>            NS_ERROR("unable to determine content length");
>            return NS_ERROR_FAILURE;
>          }
> +        // contentLength64 is under PR_INT32_MAX
> +        contentLength = PRInt32(contentLength64);

This is non-sense.  I really don't think using this method (a "32-bits" method) should limit to 32 bits of length when aContentLength is < 0.

@@ +492,4 @@
>        }
>        // SetRequestHeader propagates headers to chrome if HttpChannelChild 
>        nsCAutoString contentLengthStr;
>        contentLengthStr.AppendInt(PRInt64(contentLength));

Here use the contentLength64.

@@ +524,5 @@
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    PRUint64 streamLength = 0;
> +    if (NS_SUCCEEDED(aStream->Available(&streamLength))) {
> +      aContentLength = streamLength;

This can go to negative values when streamLength is over 1<<63 (however unlikely).

Use PRUint64 var to operate with.  if aContentLength < 0 then read the stream's Available to it.  Otherwise assign aContentLength to it.  Then you can use it bellow, also for creating the Content-Length header value.

@@ +530,3 @@
>      if (aContentLength < 0) {
>        NS_ERROR("unable to determine content length");
>        return NS_ERROR_FAILURE;

This must fail only when call to Available() failed (NS_FAILED).

Giving r- based on these changes.
Attachment #563032 - Flags: review?(honzab.moz) → review-
Thanks, Honza.

>::: extensions/pref/autoconfig/src/nsReadConfig.cpp
>@@ +316,5 @@
>> +    PRUint64 fs64;
>> +    rv = inStr->Available(&fs64);
>> +    NS_ENSURE_SUCCESS(rv, rv);
>> +    // XXX no support over 4GB
>> +    NS_ENSURE_TRUE(fs64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);
>
> Out of scope of this bug: 64bit build should be able to allocate >4GB.  The check
> should be for ~PRSize(0), IMO.  But it doesn't belong here.

Should we support over 4GB config file?  Most situation is that it is corrupted.


>::: netwerk/protocol/http/HttpBaseChannel.cpp
>@@ +477,5 @@
>>      if (contentType.IsEmpty()) {
>>        mUploadStreamHasHeaders = PR_TRUE;
>>        mRequestHead.SetMethod(nsHttp::Post); // POST request
>>      } else {
>>        if (contentLength < 0) {
>
>I think we will have to change this API, len == 0 means "do get length your self",
>instead of len == -1.

I think we should support Content-Length: 0 case.  How do you think?
(In reply to Makoto Kato from comment #43)
> Should we support over 4GB config file?  Most situation is that it is
> corrupted.

Depends, we can report this as a separate bug.  But that is really minor.

> I think we should support Content-Length: 0 case.  How do you think?

Yes, I have to think more about this particular piece of code first.  The answer will be in the final review.
Makoto - we'd like to finish this soon (in the next week or two), are you able to finish it or should we assign an engineer to do it?
(In reply to Josh Aas (Mozilla Corporation) from comment #45)
> Makoto - we'd like to finish this soon (in the next week or two), are you
> able to finish it or should we assign an engineer to do it?

I told Makato to let me first finish the review.  I plan to do it soon with other large reviews I have on the list.
I have to split patch for review.
Comment on attachment 563032 [details] [diff] [review]
fix v5

Review of attachment 563032 [details] [diff] [review]:
-----------------------------------------------------------------

So, here is the rest of the review.  Sorry for the delay, but this is quit big.

This needs a test... at least manually check it works.

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +470,5 @@
>          avail = 0;
>      }
>      else if (NS_SUCCEEDED(rv) && avail) {
>          // figure out how much data to report (XXX detect overflow??)
> +        if (avail + mStreamOffset > mStreamLength)

This should be if (avail > mStreamLength - mStreamOffset)

::: netwerk/base/src/nsSyncStreamListener.cpp
@@ +202,1 @@
>      mStatus = mPipeIn->ReadSegments(writer, closure, avail, result);

Just curious why you don't use the same style of your changes as in Read() method of this class.

::: netwerk/ipc/NeckoMessageUtils.h
@@ +209,5 @@
>      if (!serializable) {
>        NS_WARNING("nsIInputStream implementation doesn't support nsIIPCSerializable; falling back to copying data");
>  
>        nsCString streamString;
> +      PRUint64 bytes = 0;

No need to init IMO.

To clean this code up a bit (not for this bug): better to check on result of mStream->Available call, and if success && bytes > 0 or a failure different from NS_BASE_STREAM_CLOSED, treat it as bytes > 0.

@@ +214,3 @@
>  
>        aParam.mStream->Available(&bytes);
> +      NS_ABORT_IF_FALSE(bytes > PR_UINT32_MAX, "nsIInputStream has over 4GB data");

Please add a comment why you don't want to all this.

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +55,5 @@
>  #include "nsIMIMEService.h"
>  
> +#ifndef PR_INT64_MAX
> +#define PR_INT64_MAX (~((PRInt64)(1) << 63))
> +#endif

Also on more places in this patch, what is the reason not to include "prtypes.h" here?  I don't say this is wrong, just please add a comment why rather define this.

@@ +433,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
> +      NS_ENSURE_TRUE(avail <= PR_INT64_MAX, NS_ERROR_FAILURE);

FILE_TOO_BIG?

::: netwerk/protocol/ftp/nsFtpControlConnection.cpp
@@ +77,2 @@
>          if (NS_SUCCEEDED(rv) && n != avail)
>              avail = n;

I'd change this as:

if (NS_SUCCEEDED(rv))
   avail = n;

Otherwise you should cast n or avail in the n != avail condition.

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +457,5 @@
>          return mStatus;
>      }
>  
>      nsresult rv;
> +    PRUint64 avail = 0;

Also cast avail bellow in this method in call to ReadSegments.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +445,3 @@
>      if (NS_FAILED(mRequestStream->Available(&size)))
>          size = 0;
> +    return size > PR_UINT32_MAX ? PR_UINT32_MAX : (PRUint32)size;

Hmm... this API (nsAHttpTransaction::Available()) should for consistency be changed to PRUint64 result as well.  This simple change is allowed at this moment only because the consumers are not expecting the real size and are ready to get a 32-bits clipped value.

This should be changed in a follow bug.  Please CC Patrick McManus to that bug.

::: netwerk/streamconv/test/Converters.cpp
@@ +95,5 @@
>      if (NS_FAILED(rv)) return rv;
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);

Why added ?

@@ +96,5 @@
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return mListener->OnDataAvailable(request, ctxt, convertedStream, sourceOffset, PRUint32(NS_MIN(len, PRUint64(PR_UINT32_MAX))));

Also, in case Available would return something more then PR_UINT32_MAX, you must loop this ODA call until all data gets consumed from convertedStream (i.e. push all <=4GB pieces one by one) or ODA returns a failure.

As I understand, if this would be a last call to 	TestConverter::OnDataAvailable, then not all data would get to mListener when there would be over 4GB data in convertedStream.

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +140,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    PRUint64 avail;
> +    rv = dataStream->Available(&avail);
> +    NS_ENSURE_SUCCESS(rv, rv);

Why added?

@@ +145,3 @@
>  
> +    return aListener->OnDataAvailable(request, nsnull, dataStream, 0,
> +                                      PRUint32(NS_MIN(avail, PRUint64(PR_UINT32_MAX))));

The same, you have to loop all 4GB chunks to aListener.

::: rdf/base/src/nsRDFXMLDataSource.cpp
@@ +574,2 @@
>          if (NS_SUCCEEDED(rv))
>              offset += avail;

Offset should be also PRUint64 and clamped at 4GB, as in SAX.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2969,5 @@
>  
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
> +  PRUint64 len = aLen == PR_UINT32_MAX ? n : aLen;

PRUint64 len = (aLen == PR_UINT32_MAX) ? n : aLen;

::: startupcache/StartupCacheUtils.cpp
@@ +120,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(avail64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +2308,5 @@
>        break; // blocking input stream has none available when done
>  
>      rv = listener->OnDataAvailable(mImportChannel, nsnull, bufferedstream, 0,
> +                                   available > PR_UINT32_MAX ?
> +                                     PR_UINT32_MAX : available);

Separate this ternary operator to its own line, don't forget to cast available to PRUint32.

::: widget/src/windows/JumpListBuilder.cpp
@@ +655,4 @@
>    rv = iconStream->Available(&bufSize);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  // XXX no support over 4GB stream
> +  NS_ENSURE_TRUE(bufSize <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

@@ +660,5 @@
>    // Setup a buffered output stream from the stream object
>    // so that we can simply use WriteFrom with the stream object
>    nsCOMPtr<nsIOutputStream> bufferedOutputStream;
>    rv = NS_NewBufferedOutputStream(getter_AddRefs(bufferedOutputStream),
>                                    outputStream, bufSize);

Cast

::: xpcom/components/nsComponentManager.cpp
@@ +556,5 @@
>  
> +    PRUint64 flen64;
> +    nsresult rv = is->Available(&flen64);
> +    NS_ENSURE_SUCCESS(rv, );
> +    NS_ENSURE_TRUE(flen64 < PR_UINT32_MAX, ); // no support 4GB or over

Just return, this soon won't build.  If you want to log, use NS_ERROR.  In case of 4GB limit I think it would be worth to.

@@ +581,1 @@
>              return;

The check above should be (just) for PRUint64(totalRead) + avail < flen64.  You must protect not going past the the allocated memory.  It could happen when Available of the stream would grow.  It is actually an already present bug in this not well written code.

::: xpcom/io/Base64.cpp
@@ +196,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // no support 4GB or over
> +    NS_ENSURE_TRUE(count < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);
> +    aCount = PRUint32(count);

You'll have to update this, see http://hg.mozilla.org/mozilla-central/rev/aa47e51cbd8a

::: xpcom/io/nsPipe3.cpp
@@ +743,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

I'm not sure you need to change mAvailable to be 64-bits here.  The limit for the pipe's buffer is 4GB anyway.  See the nsPipe::Init method and maxCount = PRUint32(-1) / segmentSize math.

You may just return a casted mAvailable here I think.

This is sensitive code, I would like to put minimal changes to it.

::: xpcom/io/nsScriptableInputStream.cpp
@@ +59,5 @@
>  nsScriptableInputStream::Available(PRUint32 *_retval) {
>      if (!mInputStream) return NS_ERROR_NOT_INITIALIZED;
> +    PRUint64 avail;
> +    nsresult rv = mInputStream->Available(&avail);
> +    NS_ENSURE_SUCCESS(rv, rv);

Just return rv please, this is not a fatal error here.  Throw indicates a valid state of the stream.

@@ +71,5 @@
>      PRUint32 count = 0;
>      char *buffer = nsnull;
>  
> +    rv = Available(&count);
> +    NS_ENSURE_SUCCESS(rv, rv);

Just return rv please.  This adds an error log, seems the original coder intended to not pollute error output.

@@ +76,3 @@
>  
>      count = NS_MIN(count, aCount);
>      buffer = (char*)nsMemory::Alloc(count+1); // make room for '\0'

Please see bug 716556.

::: xpcom/io/nsStreamUtils.cpp
@@ +650,5 @@
>                  rv = NS_OK;
>              break;
>          }
>          if (avail == 0)
>              break;

You should cast avail on all places or, what I prefer, just temporarily keep it in some avail64 and then cast to avail after check for (avail > maxCount).

::: xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp
@@ +166,5 @@
>      return header;
>  }
>  
>  XPTHeader*
>  xptiInterfaceInfoManager::ReadXPTFileFromInputStream(nsIInputStream *stream)

This method had been removed completely.  Just for reference:
https://bugzilla.mozilla.org/attachment.cgi?id=579975&action=diff#a/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp_sec1

Now the file is read directly using NSPR:
http://mxr.mozilla.org/mozilla-central/source/xpcom/build/FileLocation.cpp#177

So no worries about searching an Available consumer to search for ;)
(In reply to Honza Bambas (:mayhemer) from comment #44)
> > I think we should support Content-Length: 0 case.  How do you think?
> 
> Yes, I have to think more about this particular piece of code first.  The
> answer will be in the final review.

You are right, leave it.
Depends on: 720627
Depends on: 720957
Attached patch Part 1. IDL changes (obsolete) — Splinter Review
Attachment #563032 - Attachment is obsolete: true
Attached patch Part4. libpref part (obsolete) — Splinter Review
Attached patch Part 5. xpconnect part (obsolete) — Splinter Review
Attached patch Part 6. libjar part (obsolete) — Splinter Review
Attached patch Part 7. rdf part (obsolete) — Splinter Review
Attached patch part 8. parser part (obsolete) — Splinter Review
Attached patch part 9. gfx part (obsolete) — Splinter Review
Attachment #591733 - Attachment description: gfx part → part 9. gfx part
Attached patch Part 10. image part (obsolete) — Splinter Review
Attached patch Part 11. dom part (obsolete) — Splinter Review
Attached patch Part 12. widget part (obsolete) — Splinter Review
Attached patch Part 13. content part (obsolete) — Splinter Review
Attached patch Part 2. xpcom part (obsolete) — Splinter Review
Attached patch Part 14. toolkit part (obsolete) — Splinter Review
Attached patch Part 15. security part (obsolete) — Splinter Review
Attached patch Part 17. startupcache (obsolete) — Splinter Review
Attached patch Part 16. extension part (obsolete) — Splinter Review
Attachment #591713 - Flags: review?(honzab.moz)
Attachment #591732 - Flags: review?(honzab.moz)
Attachment #591728 - Flags: review?(honzab.moz)
Attachment #591721 - Flags: review?(honzab.moz)
Attachment #591733 - Flags: review?(honzab.moz)
Attachment #591737 - Flags: review?(honzab.moz)
Attachment #591741 - Flags: review?(honzab.moz)
Attachment #591745 - Flags: review?(honzab.moz)
Attachment #591750 - Flags: review?(honzab.moz)
Attachment #592044 - Flags: review?(honzab.moz)
Attachment #592010 - Flags: review?(honzab.moz)
Attachment #592012 - Flags: review?(honzab.moz)
Attached patch Part 8. parser part (obsolete) — Splinter Review
Attachment #591732 - Attachment is obsolete: true
Attachment #592059 - Flags: review?(honzab.moz)
Attachment #591732 - Flags: review?(honzab.moz)
Attached patch Part 5. xpconnect part (obsolete) — Splinter Review
Attachment #591725 - Attachment is obsolete: true
Attachment #592061 - Flags: review?(honzab.moz)
Attachment #592009 - Flags: review?(honzab.moz)
Attachment #592061 - Attachment is patch: true
Attached patch Part 3. network part (obsolete) — Splinter Review
waiting for bug 720627 ...
Attached patch Part 6. libjar part (obsolete) — Splinter Review
Attachment #591727 - Attachment is obsolete: true
Attachment #592072 - Flags: review?(honzab.moz)
Attachment #592045 - Flags: review?(honzab.moz)
BTW, I would prefer a merged patch (instead/besides).  I like to compare patches directly, this makes it very hard.  But for this round let's go with this.
Makato, I'm missing changes to layout/base/nsPresShell.cpp.
And also to xpcom/components/nsComponentManager.cpp
Adding Jeff, I'd like to extend httpd.js to support or at least accept and intercept uploads with >32 bit length.
Comment on attachment 591713 [details] [diff] [review]
Part 1. IDL changes

Review of attachment 591713 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

I'd like Benjamin to check on the change to nsIScriptableInputStream.
Attachment #591713 - Flags: review?(honzab.moz)
Attachment #591713 - Flags: review?(benjamin)
Attachment #591713 - Flags: review+
Comment on attachment 592062 [details] [diff] [review]
Part 3. network part

Review of attachment 592062 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab, preliminarily giving r+.  But before an official one I want to test this.  Adding some simple support to httpd.js and try creating a test is IMO the best way.  Not many widely used servers now support >4G uploads.

::: netwerk/protocol/device/AndroidCaptureProvider.cpp
@@ +138,4 @@
>  {
>    mozilla::ReentrantMonitorAutoEnter autoMonitor(mMonitor);
>  
>    *aAvailable = mAvailable;

Cast?

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +429,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
>        mUploadLength = avail;

Here you were checking for avail < INT64_MAX.  It should be preserved.  We should never have mUploadStream && mUploadLength < 0.  It would break logic in nsFileCopyEvent::DoCopy().

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +483,5 @@
> +       hasHeaders = true;
> +     } else {
> +       method = nsHttp::Put;
> +       hasHeaders = false;
> +     }

Indention.

@@ +491,5 @@
> +
> +  // if stream is null, ExplicitSetUploadStream returns error.
> +  // So we need special case for GET method.
> +  mUploadStreamHasHeaders = false;
> +  mRequestHead.SetMethod(nsHttp::Get); // revert to GET request

Nice!

@@ +511,5 @@
>    // Ensure stream is set and method is valid 
>    NS_ENSURE_TRUE(aStream, NS_ERROR_FAILURE);
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    aStream->Available((PRUint64*)&aContentLength);

This should be static cast.

Also I think we really should add an NS_FAILED(rv) check here.

If call to Available fails, then calls to any method of the stream fail.

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +738,5 @@
>  
>      while ((trans = Request(0)) != nsnull) {
>          avail = trans->Available();
>          if (avail) {
> +            rv = trans->ReadSegments(this, (PRUint32)NS_MIN(avail, (PRUint64)PR_UINT32_MAX), &n);

Also according the 80 lines limit, please separate the NS_MIN evaluation to its own line.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +167,5 @@
>      nsCOMPtr<nsIHttpActivityObserver> mActivityDistributor;
>  
>      nsCString                       mReqHeaderBuf;    // flattened request headers
>      nsCOMPtr<nsIInputStream>        mRequestStream;
> +    PRUint64                        mRequestSize;

You may also remove the comment about 32 bits in nsHttpTransaction::OnTransportStatus

::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp
@@ +125,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    streamLen = (PRUint32)NS_MIN(streamLen64, PRUint64(PR_UINT32_MAX - 1));

Nice, one I didn't catch before ;)

::: netwerk/streamconv/test/Converters.cpp
@@ +104,1 @@
>      convertedStream->Available(&len);

Ah, I didn't realize previously you wanted to check on validity of |len|.  But this is probably OK for the testing code.

@@ +106,5 @@
> +    PRUint64 offset = sourceOffset;
> +    while (len > 0) {
> +        PRUint32 count = saturated(len);
> +        rv = mListener->OnDataAvailable(request, ctxt, convertedStream, saturated(offset), count);
> +        NS_ENSURE_SUCCESS(rv, rv);

Yes, but please only if (NS_FAILED(rv)) return rv;

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +150,5 @@
> +    while (avail > 0) {
> +        PRUint32 count = saturated(avail);
> +        rv = aListener->OnDataAvailable(request, nsnull, dataStream,
> +                                        saturated(offset), count);
> +        NS_ENSURE_SUCCESS(rv, rv);

Same here, just return.
Attachment #592062 - Flags: review+
Comment on attachment 591721 [details] [diff] [review]
Part4. libpref part

Review of attachment 591721 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #591721 - Flags: review?(honzab.moz) → review+
Comment on attachment 592061 [details] [diff] [review]
Part 5. xpconnect part

Review of attachment 592061 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +910,2 @@
>              NS_ENSURE_SUCCESS(rv, rv);
> +            NS_ENSURE_TRUE(len64 <= PR_UINT32_MAX, NS_ERROR_FILE_TOO_BIG);

The condition was correct :)  please change it to |<|.
Attachment #592061 - Flags: review?(honzab.moz) → review+
Comment on attachment 592072 [details] [diff] [review]
Part 6. libjar part

Review of attachment 592072 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with the comments addressed.

::: modules/libjar/nsJAR.cpp
@@ +469,2 @@
>    if (NS_FAILED(rv)) return rv;
> +  NS_ENSURE_TRUE(len64 <= PR_UINT32_MAX, NS_ERROR_FILE_CORRUPTED); // bug 164695

Again, this was correct, change the condition to <.
Attachment #592072 - Flags: review?(honzab.moz) → review+
Comment on attachment 591728 [details] [diff] [review]
Part 7. rdf part

Review of attachment 591728 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #591728 - Flags: review?(honzab.moz) → review+
Comment on attachment 592059 [details] [diff] [review]
Part 8. parser part

Review of attachment 592059 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #592059 - Flags: review?(honzab.moz) → review+
Comment on attachment 591733 [details] [diff] [review]
part 9. gfx part

Review of attachment 591733 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with comments addressed

::: gfx/thebes/gfxASurface.cpp
@@ +802,4 @@
>    if (NS_FAILED(rv))
>      return;
>  
> +  if (bufSize64 - 16 > PR_UINT32_MAX)

This is wrong, I think.  The check has to be (bufSize64 > PR_UINT32_MAX - 16)

@@ +803,5 @@
>      return;
>  
> +  if (bufSize64 - 16 > PR_UINT32_MAX)
> +    return;
> +  PRUint32 bufSize = (PRUint32)bufSize64;

Blank line before this one please.
Attachment #591733 - Flags: review?(honzab.moz) → review+
Comment on attachment 591737 [details] [diff] [review]
Part 10. image part

Review of attachment 591737 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #591737 - Flags: review?(honzab.moz) → review+
Comment on attachment 591741 [details] [diff] [review]
Part 11. dom part

Review of attachment 591741 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #591741 - Flags: review?(honzab.moz) → review+
Comment on attachment 591745 [details] [diff] [review]
Part 12. widget part

Review of attachment 591745 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #591745 - Flags: review?(honzab.moz) → review+
Comment on attachment 591750 [details] [diff] [review]
Part 13. content part

Review of attachment 591750 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with the comments addressed

::: content/base/src/nsFrameMessageManager.cpp
@@ +796,2 @@
>      nsCString buffer;
> +    PRUint32 avail = (PRUint32)NS_MIN(avail64, (PRUint64)PR_UINT32_MAX);

Hmm... hard to say what is right here.  But I think we should fail with FILE_TOO_BIG if avail64 > PR_UINT32_MAX.  Processing just part of the data is IMO not correct.
Attachment #591750 - Flags: review?(honzab.moz) → review+
Comment on attachment 592010 [details] [diff] [review]
Part 14. toolkit part

Review of attachment 592010 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: toolkit/components/places/nsFaviconService.cpp
@@ +644,1 @@
>      return NS_ERROR_FAILURE;

Good!  You just may want to return NS_ERROR_FILE_TOO_BIG for (available64 > PR_UINT32_MAX / sizeof(PRUint8))
Attachment #592010 - Flags: review?(honzab.moz) → review+
Comment on attachment 592012 [details] [diff] [review]
Part 15. security part

Review of attachment 592012 [details] [diff] [review]:
-----------------------------------------------------------------

r-

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2777,5 @@
>    nsresult rv = data->Available(&n);
>    if (NS_FAILED(rv))
>      return rv;
> +  if (n > PR_UINT32_MAX)
> +    return NS_ERROR_FILE_TOO_BIG;

No, you have to respect the len argument.  We should actually read MIN(n, len).

Please file a followup to change this API to 64 bit.

@@ +2977,5 @@
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
>    if (aLen == PR_UINT32_MAX)
> +    aLen = (PRUint32)n;

Same here..
Attachment #592012 - Flags: review?(honzab.moz) → review-
Comment on attachment 592045 [details] [diff] [review]
Part 16. extension part

Review of attachment 592045 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
@@ +465,5 @@
>        // Update the content length attribute on the channel.  We do this
>        // synchronously without proxying.  This hack is not as bad as it looks!
> +      if (mBytesRemaining != PR_UINT64_MAX)
> +        // XXX 64-bit
> +        mChannel->SetContentLength((PRInt32)mBytesRemaining);

Clamp to PR_UINT32_MAX for now?  This definitely needs a followup bug, can you file one please?

Braces around the two-lines of comment + command.

(My previous review comment on this line was wrong, btw!)
Attachment #592045 - Flags: review?(honzab.moz) → review+
Comment on attachment 592044 [details] [diff] [review]
Part 17. startupcache

Review of attachment 592044 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #592044 - Flags: review?(honzab.moz) → review+
Comment on attachment 592009 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 592009 [details] [diff] [review]:
-----------------------------------------------------------------

Left r?.

::: xpcom/io/nsPipe3.cpp
@@ +137,5 @@
>  
>      nsresult Fill();
>      void SetNonBlocking(bool aNonBlocking) { mBlocking = !aNonBlocking; }
>  
> +    PRUint64 Available() { return mAvailable; }

Cast.

However, why are you adding this change?

@@ +744,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

Cast

::: xpcom/io/nsScriptableInputStream.cpp
@@ +73,3 @@
>      if (NS_FAILED(rv)) return rv;
>  
> +    PRUint32 count = (PRUint32)NS_MIN(count64, (PRUint64)aCount);

This still waits for bug 716556...
Comment on attachment 591713 [details] [diff] [review]
Part 1. IDL changes

This is certainly ok from an XPCOM perspective. I'm a little worried that we might end up with JS issues because JS numbers don't have full 64-bit precision. Is that something we need to audit for somehow?
Attachment #591713 - Flags: review?(benjamin) → review+
In practice, that will only be an issue for things that are > 53 bits, right?  That gets us up to 8 petabytes or so.  Probably safe for a few years...
(In reply to Honza Bambas (:mayhemer) from comment #72)
> And also to xpcom/components/nsComponentManager.cpp

By bug 695843 (changeset 502c67d69baa), nsIInputStream->Available() usages is removed.
Attached patch Part 18. layout part (obsolete) — Splinter Review
Attachment #596577 - Attachment is patch: true
Attachment #596577 - Flags: review?(honzab.moz)
Comment on attachment 596577 [details] [diff] [review]
Part 18. layout part

Review of attachment 596577 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Attachment #596577 - Flags: review?(honzab.moz) → review+
Comment on attachment 592009 [details] [diff] [review]
Part 2. xpcom part

I think you can now merge this to a patch in bug 716556.
Attachment #592009 - Flags: review?(honzab.moz)
Makato, any updates from you?  Or should someone else take your work and finish it instead?
Attached patch Part 19. for comm-central (obsolete) — Splinter Review
Attachment #607540 - Flags: review?(dbienvenu)
Attached patch Part 2. xpcom part (obsolete) — Splinter Review
Attachment #592009 - Attachment is obsolete: true
Attachment #607541 - Attachment is patch: true
Attachment #607541 - Flags: review?(honzab.moz)
Comment on attachment 607541 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 607541 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't address my requirements from comment 90.


This applies to all my review comments for all patches ("r+ with comments" means there are things to update and I trust you do them).  Please address or discuss them before landing all the patches.
(In reply to Honza Bambas (:mayhemer) from comment #100)
> This applies to all my review comments for all patches ("r+ with comments"
> means there are things to update and I trust you do them).  Please address
> or discuss them before landing all the patches.

oh, I have missed comment #90... I will update tomorrow.
Attachment #607541 - Flags: review?(honzab.moz)
Attached patch Part 15. security part (obsolete) — Splinter Review
before this, I have to fix updateFromStream makes 64bit.  if len param is just 4GB, it isn't clear whether 4GB is unknown or just size.
Attachment #592012 - Attachment is obsolete: true
Blocks: 737451
No longer blocks: 737451
Depends on: 737451
Comment on attachment 607540 [details] [diff] [review]
Part 19. for comm-central

looks ok, but can you wrap some of the long lines at 80 chars? thx!

-      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(avail, mSuspendedReadBytes), &amountWritten);
+      m_outputStream->WriteFrom(mPostDataStream, (PRUint32)NS_MIN<PRUint64>(avail, mSuspendedReadBytes), &amountWritten);

-      mOutListener->OnDataAvailable(mChannel, mURL, mInputStream, 0, avail);
+      mOutListener->OnDataAvailable(mChannel, mURL, mInputStream, 0, (PRUint32)NS_MIN<PRUint64>(avail, PR_UINT32_MAX));

-      rv2 = mOutListener->OnDataAvailable(request, mURL, mInputStream, 0, bytesInStream);
+      mOutListener->OnDataAvailable(request, mURL, mInputStream, 0, (PRUint32)NS_MIN<PRUint64>(bytesInStream, PR_UINT32_MAX));

-        m_channelListener->OnDataAvailable(this, m_channelContext, mDisplayInputStream, 0, inlength);
+        m_channelListener->OnDataAvailable(this, m_channelContext, mDisplayInputStream, 0, (PRUint32)NS_MIN<PRUint64>(inlength, PR_UINT32_MAX));
Attachment #607540 - Flags: review?(dbienvenu) → review+
Comment on attachment 607541 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 607541 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsStreamUtils.cpp
@@ +638,3 @@
>  
>  nsresult
>  NS_ConsumeStream(nsIInputStream *stream, PRUint32 maxCount, nsACString &result)

Actually, Makato, maxCount should change here to 64 bit as well.

If you change OnDataAvailable to be 64 bit, then you'll hit this anyway.
Attached patch Part 2. xpcom part v4 (obsolete) — Splinter Review
Attachment #607541 - Attachment is obsolete: true
Comment on attachment 608640 [details] [diff] [review]
Part 2. xpcom part v4

Although NS_ConsumeStream changes to 64-bit, nsString cannot handle over 4GB length.  I add overflow check to it.
Attachment #608640 - Flags: review?(honzab.moz)
Comment on attachment 608640 [details] [diff] [review]
Part 2. xpcom part v4

Review of attachment 608640 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsStreamUtils.cpp
@@ +641,3 @@
>  {
> +    // XXX nsIInputStearm supports 64-bit length stream, but nsString support
> +    // until 4GB 

Aggrhh... I'm sorry.  I didn't realize this consumes to nsACString... 

As I see this code now, I realize it's a bad advice to make maxCount 64 bit...


e.g. nsStreamCipher::UpdateFromStream that delegates to consume from string (quit strange implementation btw), should call NS_ConsumeStream+UpdateFromString in loops for max 32bit lengths, similar to as you do for OnDataAvailable on some places.

This method's signature has to stay the same...

@@ +661,5 @@
>          PRUint32 length = result.Length();
> +        // nsString cannot handle over 4GB length.
> +        if (length + avail64 > PR_UINT32_MAX)
> +            return NS_ERROR_FILE_TOO_BIG;
> +        PRUint32 avail = (PRUint32)avail64;

The code at [1] was correct then.  Please just add similar check as you do here but in form:

if (avail > PR_UINT32_MAX - length)
  return NS_ERROR_OUT_OF_MEMORY;

Subtract since (avail + length) may overflow: this will prevent potential security issue with buffer overflow when we consume more then just a single stream to just a single string and length of the streams together may overlap 2^32.

There is currently no place in the platform code that would do that.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=607541&action=diff#a/xpcom/io/nsStreamUtils.cpp_sec1
Attachment #608640 - Flags: review?(honzab.moz)
Attachment #633976 - Attachment description: part 19. dom/devicestorage part → Part 20. dom/devicestorage part
Attached patch Part 2. xpcom part v4.1 (obsolete) — Splinter Review
Attachment #608640 - Attachment is obsolete: true
Attachment #633977 - Flags: review?(honzab.moz)
Attachment #633976 - Flags: review?(honzab.moz)
Attached patch Part 15. security part v2 (obsolete) — Splinter Review
Attachment #607570 - Attachment is obsolete: true
Attachment #633979 - Flags: review?(honzab.moz)
Comment on attachment 633977 [details] [diff] [review]
Part 2. xpcom part v4.1

Review of attachment 633977 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of a non-addressed comment.

::: xpcom/io/nsStreamUtils.cpp
@@ +662,3 @@
>          result.SetLength(length + avail);
>          if (result.Length() != (length + avail))
>              return NS_ERROR_OUT_OF_MEMORY;

You didn't address my comment 107 here.

Add also check before call to SetLength on the result:

if (avail > PR_UINT32_MAX - length)
  return NS_ERROR_OUT_OF_MEMORY;
Attachment #633977 - Flags: review?(honzab.moz) → review-
Comment on attachment 633979 [details] [diff] [review]
Part 15. security part v2

Review of attachment 633979 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab.

That's it, thanks.
Attachment #633979 - Flags: review?(honzab.moz) → review+
Comment on attachment 633976 [details] [diff] [review]
Part 20. dom/devicestorage part

Review of attachment 633976 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with a nit.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +787,5 @@
>      }
>  
> +    while (bufSize) {
> +      PRUint32 wrote;
> +      rv = bufferedOutputStream->WriteFrom(stream, static_cast<PRUint32>(NS_MIN<PRUint64>(bufSize, PR_UINT32_MAX)), &wrote);

Nicer would be to have a separate local var PRUint32 read = stream, static_cast<PRUint32>(NS_MIN<PRUint64>(bufSize, PR_UINT32_MAX));  This is hard to read.
Attachment #633976 - Flags: review?(honzab.moz) → review+
Taking this bug now since there is no activity and we want to get this done.  This is nearly done, just unbitrott (hopefully no major changes needed) and get some last part for XPCOM reviewed.
Assignee: m_kato → honzab.moz
Status: NEW → ASSIGNED
Comment on attachment 607540 [details] [diff] [review]
Part 19. for comm-central

Major issues in this bug, r-.
Attachment #607540 - Flags: review+ → review-
Attached patch Merged WIP 1 (obsolete) — Splinter Review
For backup, folded, rebased and buildable patch.  Not tested so far.
Attachment #591713 - Attachment is obsolete: true
Attachment #591721 - Attachment is obsolete: true
Attachment #591728 - Attachment is obsolete: true
Attachment #591733 - Attachment is obsolete: true
Attachment #591737 - Attachment is obsolete: true
Attachment #591741 - Attachment is obsolete: true
Attachment #591745 - Attachment is obsolete: true
Attachment #591750 - Attachment is obsolete: true
Attachment #592010 - Attachment is obsolete: true
Attachment #592044 - Attachment is obsolete: true
Attachment #592045 - Attachment is obsolete: true
Attachment #592059 - Attachment is obsolete: true
Attachment #592061 - Attachment is obsolete: true
Attachment #592062 - Attachment is obsolete: true
Attachment #592072 - Attachment is obsolete: true
Attachment #596577 - Attachment is obsolete: true
Attachment #607540 - Attachment is obsolete: true
Attachment #633976 - Attachment is obsolete: true
Attachment #633977 - Attachment is obsolete: true
Attachment #633979 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This bitrotts so fast!  Merged again and this time also testes with 64 patched PHP

r=honzab except for xpcom parts.  Subpatch for just those parts will follow.
Attachment #649259 - Attachment is obsolete: true
Attachment #650904 - Flags: review+
r=honzab except the xpcom parts.
Attachment #650904 - Attachment is obsolete: true
Attachment #650929 - Flags: review+
Only change in the xpcom part patch I was missing and hence gave r-.  

This should get reviewed by some xpcom peer, extremely simple patch, tho.
Attachment #650935 - Flags: review?(benjamin)
This is contained in the "v1 with my review comments addressed" patch.  It is actually just an interdiff after my review+ comments were addressed (by me).

Probably, respected peers for each module should re-review this as well, some of the changes are of the same weight as the missing xpcom change, that I asked to r from Benjamin.


Josh (or anyone), may I ask you to arrange those reviews, or just land this as is?
Attachment #650940 - Flags: review?
Attachment #650940 - Attachment description: my "r+ with comments" addressed → interdiff: my "r+ with comments" addressed
Temporarily taking this bug so I remember to arrange reviews and get this landed.
Assignee: honzab.moz → joshmoz
Attachment #650940 - Flags: review?(jonas)
Attachment #650940 - Flags: review?(bgirard)
Attachment #650940 - Flags: review?
Attached patch comm-central v2 (obsolete) — Splinter Review
No idea who should review this.
Attachment #650947 - Flags: review?
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

Review of attachment 650940 [details] [diff] [review]:
-----------------------------------------------------------------

stealing review from BenWa
Attachment #650940 - Flags: review?(bgirard) → review+
Attachment #650935 - Flags: review?(benjamin) → review+
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

Review of attachment 650940 [details] [diff] [review]:
-----------------------------------------------------------------

Just one non-critical nit.

::: xpcom/io/nsStreamUtils.cpp
@@ +626,5 @@
>          // resize result buffer
>          PRUint32 length = result.Length();
> +        if (avail > PR_UINT32_MAX - length)
> +            return NS_ERROR_FILE_TOO_BIG;
> +        

There's some extra whitespace here.
Attachment #650940 - Flags: review?(jonas) → review+
Attachment #650947 - Flags: review? → review?(mozilla)
One more try run before I land it:

https://tbpl.mozilla.org/?tree=Try&rev=4b1a019515e3
Assignee: joshmoz → honzab.moz
Thanks for the reviews and finishing this Josh!

A note about how I was testing this:
- Fedora x64 box
- Apache x64
- PHP x64 with patch for https://bugs.php.net/bug.php?id=44522 (from 2012-03-26 03:59) applied
- limits for PHP upload raised to some 9GB, as well as script timeout
- simple PHP upload form
- Win32 build of Firefox on win7 x64
- uploading some 7.5GB file
- sha1 checksum confirmed the file has arrived correctly to the server
https://hg.mozilla.org/mozilla-central/rev/14e988e17b79
Status: ASSIGNED → RESOLVED
Closed: 21 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Blocks: 782102
(In reply to Ryan VanderMeulen from comment #129)
> https://hg.mozilla.org/mozilla-central/rev/14e988e17b79

This landing seems to have completely broken comm-central builds for both Thunderbird and SeaMonkey:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/util/nsMsgLineBuffer.cpp: In member function ‘char* nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream*, PRUint32&, bool&, nsresult*, bool)’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/util/nsMsgLineBuffer.cpp:324:51: error: no matching function for call to ‘nsIInputStream::Available(PRUint32*)’
../../../mozilla/dist/include/nsIInputStream.h:66:60: note: candidate is: virtual nsresult nsIInputStream::Available(PRUint64*)
NEXT ERROR make[6]: *** [nsMsgLineBuffer.o] Error 1
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/src/nsMessenger.cpp: In member function ‘virtual nsresult nsSaveMsgListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, PRUint32, PRUint32)’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/src/nsMessenger.cpp:1908:40: error: no matching function for call to ‘nsIInputStream::Available(PRUint32*)’
../../../mozilla/dist/include/nsIInputStream.h:66:60: note: candidate is: virtual nsresult nsIInputStream::Available(PRUint64*)
NEXT ERROR make[6]: *** [nsMessenger.o] Error 1
Reopening as there are unreviewed patches that will need checking in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 650947 [details] [diff] [review]
comm-central v2

Spreading the review net slightly wider, whomever can get to it first.
Attachment #650947 - Flags: review?(neil)
Attachment #650947 - Flags: review?(mbanner)
Comment on attachment 650947 [details] [diff] [review]
comm-central v2

>-    PRUint32 numBytesToCopy = NS_MIN(numFreeBytesInBuffer - 1 /* leave one for a null terminator */, numBytesInStream);
>+    PRUint32 numBytesToCopy = (PRUint32)NS_MIN<PRUint64>(numFreeBytesInBuffer - 1 /* leave one for a null terminator */, numBytesInStream);
Bah, we could really do with uint32_t NS_MIN(uint32_t, uint64_t) and/or uint32_t NS_MIN(uint64_t, uint32_t) overloads. Or just a private version in nsMsgUtils.h would suffice.

>-          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096U), &bytesWritten);
>+          rv = aOutStream->WriteFrom(mInStream, (PRUint32)NS_MIN<PRUint64>(avail, 4096), &bytesWritten);
Can't you write 4096ULL?

>+      *pProgress = NS_MAX<PRUint64>(totalBytes - bytesLeft, PR_UINT32_MAX);
Don't you mean NS_MIN here?
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

>+    if (avail64 > PR_UINT32_MAX) {
>+      return;
>+    }
>     nsCString buffer;
>     PRUint32 avail = (PRUint32)NS_MIN(avail64, (PRUint64)PR_UINT32_MAX);
[Because of the new range check, this could have been turned into a cast...]
Keywords: dev-doc-needed
Attached patch comm-central v3 (obsolete) — Splinter Review
v3 patch for comm-central, updated with Neil's comments.
Attachment #650947 - Attachment is obsolete: true
Attachment #650947 - Flags: review?(neil)
Attachment #650947 - Flags: review?(mozilla)
Attachment #650947 - Flags: review?(mbanner)
Attachment #651342 - Flags: review?(neil)
Depends on: 782255
Attached patch comm-central v3aSplinter Review
Adds an inline statement to nsMsgUtils.h to fix linker bustage.
Attachment #651342 - Attachment is obsolete: true
Attachment #651342 - Flags: review?(neil)
Attachment #651373 - Flags: review?(neil)
Comment on attachment 651373 [details] [diff] [review]
comm-central v3a

So, having a single NS_MIN overload causes more problems than it solves, and the compiler will happily let you pass the parameters in reverse order and silently do the wrong thing. Fortunately having both overloads seems to work. (I know we're unlikely to actually see any 4GB values in mailnews, but...)

>-          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096U), &bytesWritten);
>+          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096ULL), &bytesWritten);
So, it turns out I was wrong here, because on some systems PRUint64 is long, not long long. In that case, what happens is that you end up calling your NS_MIN(uint32_t, uint64_t) overload, thus silently truncating avail to 32 bits. Oops.

>-      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(avail, mSuspendedReadBytes), &amountWritten);
>+      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(mSuspendedReadBytes, avail), &amountWritten);
With both overloads you don't have to worry about parameter order, so this can be reverted. (The other cases where the parameters are the wrong way around suffer from the same problem as above.)

>+inline uint32_t NS_MIN(uint32_t a, uint64_t b)
>+{
>+  return b < a ? b : a;
>+}
Nit: Please cast the 64-bit variable to 32-bit in case compilers start warning about it.

>+      *pProgress = NS_MIN<PRUint64>(totalBytes - bytesLeft, PR_UINT32_MAX);
Nit: don't need the <PRUint64> here. (Note that *pProgress is 32-bit, so we want the overload version anyway.)

r=me with those fixed.
Attachment #651373 - Flags: review?(neil) → review+
Comment on attachment 651373 [details] [diff] [review]
comm-central v3a

Checked in with comments addressed:

https://hg.mozilla.org/comm-central/rev/9b46fc7fd4a6

Thanks to Honza for providing the patch originally.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Why is this bug marked dev-doc-needed? What needs to be documented?
(In reply to David Bruant from comment #139)
> Why is this bug marked dev-doc-needed? What needs to be documented?

We need update the document of nsIInputStream. available method is changed.
You need to log in before you can comment on or make changes to this bug.