784912 - nsIStreamListener.onDataAvailable should handle 64-bit offset

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Makoto Kato [:m_kato]]

nsIStreamListener is unfrozen interface now.

The offset param of onDataAvailable is still 32-bit, so we should use 64-bit offset instead of.

Comment 1

[Josh Aas]

Are you planning to work on this Makoto? If not I may be able to do it.

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Josh Aas (Mozilla Corporation) from comment #1)
> Are you planning to work on this Makoto? If not I may be able to do it.

I already test this now.
https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Also, we need a patch for comm-central.

Comment 3

[Makoto Kato [:m_kato]]

aah, try server url is https://tbpl.mozilla.org/?tree=Try&rev=324dba6644b0

Comment 4

[Makoto Kato [:m_kato]]

Attachment: for m-c

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 655479 [details] [diff] [review]
for m-c

Should I separate patch (network and other)?

Comment 6

[Makoto Kato [:m_kato]]

Attachment: for c-c

Comment 7

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

Comment on attachment 655479 [details] [diff] [review]
for m-c

any specific reason to keep count a 32-bit integer, while you're touching this anyway?

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 655479 [details] [diff] [review]
for m-c

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

The patch looks good, r+ with few nits fixed.

::: content/base/src/nsDOMFileReader.cpp
@@ +299,5 @@
>    if (mDataFormat == FILE_AS_BINARY) {
>      //Continuously update our binary string as data comes in
>      NS_ASSERTION(mResult.Length() == aOffset,
>                   "unexpected mResult length");
>      uint32_t oldLen = mResult.Length();

I think here is a bug that actually has nothing to do with your patch. If (oldLen + aCount) > 0xFFFFFFFF then it overflows when passed to GetMutableData() since the argument is size_type which is uint32_t, so the string mResult is shrinked. ReadFuncBinaryString() then writes outside the buffer.

::: modules/libjar/zipwriter/src/nsDeflateConverter.cpp
@@ +101,5 @@
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext,
>   *                       in nsIInputStream aInputStream,
>   *                       in unsigned long aOffset, in unsigned long aCount); */

in unsigned long long aOffset

::: modules/libjar/zipwriter/src/nsZipDataStream.cpp
@@ +60,5 @@
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext,
>   *                       in nsIInputStream aInputStream,
>   *                       in unsigned long aOffset, in unsigned long aCount); */

in unsigned long long aOffset

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +58,5 @@
>      EndListener() {};
>  
>      // nsIStreamListener method
>      NS_IMETHOD OnDataAvailable(nsIRequest* request, nsISupports *ctxt, nsIInputStream *inStr, 
> +                               uint64_t sourceOffset, uint32_t count)

Pass offset instead of saturated(offset) in SendData() below.

Comment 9

[Makoto Kato [:m_kato]]

Attachment: for c-c

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 656337 [details] [diff] [review]
for c-c

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +349,5 @@
>  }
>  
>  // Whenever data arrives from the connection, core netlib notifices the protocol by calling
>  // OnDataAvailable. We then read and process the incoming data from the input stream.
> +NS_IMETHODIMP nsMsgProtocol::OnDataAvailable(nsIRequest *request, nsISupports *ctxt, nsIInputStream *inStr, uint64_t sourceOffset, uint32_t count)

For some reason, the build isn't failing or warning, but I think you should also update ProcessProtocolState & friends to take a uint64_t sourceOffset argument as well. Otherwise we'll be collapsing a 64 down into a 32 bit.

::: mailnews/compose/src/nsURLFetcher.cpp
@@ +474,5 @@
>  
>  /** nsIStreamListener methods **/
>  
>  /* void onDataAvailable (in nsIRequest request, in nsISupports ctxt, in nsIInputStream inStr, in unsigned long sourceOffset, in unsigned long count); */
> +NS_IMETHODIMP nsURLFetcherStreamConsumer::OnDataAvailable(nsIRequest *aRequest, nsISupports *ctxt, nsIInputStream *inStr, uint64_t sourceOffset, uint32_t count)

nit: either update the comment, or preferably, just drop it.

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +1076,5 @@
>      return NS_OK;
>  }
>  
>  /* void onDataAvailable (in nsIRequest aRequest, in nsISupports aContext, in nsIInputStream aInputStream, in unsigned long aOffset, in unsigned long aCount); */
> +NS_IMETHODIMP TokenStreamListener::OnDataAvailable(nsIRequest *aRequest, nsISupports *aContext, nsIInputStream *aInputStream, uint64_t aOffset, uint32_t aCount)

nit: either update the comment, or preferably, just drop it.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: for c-c fix v2

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 656808 [details] [diff] [review]
for c-c fix v2

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

Looks good, thanks.

Comment 13

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/mozilla-central/rev/5d63594c05a9
https://hg.mozilla.org/comm-central/rev/4618a55d2445

Comment 14

[Philip Chee]

You missed http://hg.mozilla.org/comm-central/annotate/4618a55d2445/suite/feeds/src/nsFeedSniffer.cpp#l340

Comment 15

[Philip Chee]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1346906361.1346913493.32336.gz#err0
e:/builds/slave/comm-cen-trunk-w32/build/suite/feeds/src/nsFeedSniffer.cpp(342) : error C2511: 'nsresult nsFeedSniffer::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,uint32_t,uint32_t)' : overloaded member function not found in 'nsFeedSniffer'
        e:\builds\slave\comm-cen-trunk-w32\build\suite\feeds\src\nsFeedSniffer.h(18) : see declaration of 'nsFeedSniffer'

Comment 16

[Philip Chee]

Attachment: comm-central Bustage fix.

I checked in a bustage fix r=me.
Pushed:
http://hg.mozilla.org/comm-central/rev/8ad14cba8288