nsIStreamListener.onDataAvailable should handle 64-bit offset

RESOLVED FIXED in mozilla18

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

({dev-doc-needed})

Trunk
mozilla18
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
Are you planning to work on this Makoto? If not I may be able to do it.
(Assignee)

Comment 2

5 years ago
(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=m_kato@ga2.so-net.ne.jp

Also, we need a patch for comm-central.
Assignee: nobody → m_kato
(Assignee)

Comment 3

5 years ago
aah, try server url is https://tbpl.mozilla.org/?tree=Try&rev=324dba6644b0
(Assignee)

Comment 4

5 years ago
Created attachment 655479 [details] [diff] [review]
for m-c
(Assignee)

Comment 5

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

Should I separate patch (network and other)?
Attachment #655479 - Flags: review?(honzab.moz)
(Assignee)

Comment 6

5 years ago
Created attachment 655481 [details] [diff] [review]
for c-c
Attachment #655479 - Flags: review?(honzab.moz) → review?(michal.novotny)
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 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.
Attachment #655479 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 656337 [details] [diff] [review]
for c-c
Attachment #655481 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #656337 - Flags: review?(mbanner)
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.
Attachment #656337 - Flags: review?(mbanner) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 656808 [details] [diff] [review]
for c-c fix v2
Attachment #656337 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #656808 - Flags: review?(mbanner)
Comment on attachment 656808 [details] [diff] [review]
for c-c fix v2

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

Looks good, thanks.
Attachment #656808 - Flags: review?(mbanner) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5d63594c05a9
https://hg.mozilla.org/comm-central/rev/4618a55d2445
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 14

5 years ago
You missed http://hg.mozilla.org/comm-central/annotate/4618a55d2445/suite/feeds/src/nsFeedSniffer.cpp#l340
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 15

5 years ago
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

5 years ago
Created attachment 658817 [details] [diff] [review]
comm-central Bustage fix.

I checked in a bustage fix r=me.
Pushed:
http://hg.mozilla.org/comm-central/rev/8ad14cba8288
Attachment #658817 - Flags: review+

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 789178
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.