Closed Bug 1277019 Opened 7 years ago Closed 7 years ago

header parsing methods do not account for true string length

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: deckycoss, Assigned: deckycoss)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;
58 bytes, text/x-review-board-request
mayhemer
: review+
Details 
nsHttpResponseHead::ParseHeaderLine and nsHttpTransaction::ParseLine each parse a char* without accepting an argument for the char*'s length. As a result, if a header value contains a null character, the null character and anything after it are ignored.

An example of this bug is found in the allow-headers web platform test: http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/cors/allow-headers.htm.ini

In the test, "http://web-platform.test:8000\0" is considered a match for "http://web-platform.test:8000". This is incorrect according to the W3 spec on cross-origin resource sharing (https://www.w3.org/TR/cors/#resource-sharing-check-0).
currently working on this.
Assignee: nobody → coss
Could we rewrite the two methods with mozilla::Tokenizer?
Whiteboard: [necko-would-take]
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Could we rewrite the two methods with mozilla::Tokenizer?

I think that's a good idea, but it's orthogonal to what this bug is attempting to fix.  The issue here is that we're passing a raw char* buffer around in a couple of places in this code which causes us to lose information about the length of the string.  That is what Decky is working on to fix.  I think switching to mozilla::Tokenizer would be a nice idea as a follow-up once this bug has been fixed.
are there any uses of null on the web?
Attachment #8760923 - Flags: review?(mcmanus) → review?(honzab.moz)
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

https://reviewboard.mozilla.org/r/58328/#review56398

why are the webplatform tests removed?  if those were failing and this is fixing them, we have to change the expectation, not remove the failing tests.  or am I missing something.

is there a test to check that:
- the original problem is reproducible w/o the patch and irreproducible w/ the patch
- corner cases (zero chars at the begining, end, middle, whitespaces at the start, end, only whitespaces, only zero chars)

?

If not, they must be added.  This is an easily exploitable code when wrong, so it should be tested well.

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:334
(Diff revision 1)
> +            PromiseFlatCString(line).get()));
>          return NS_ERROR_FAILURE;
>      }
>  
> +    const nsACString& sub = Substring(line, 0, split);
> +    const nsACString& sub2 = Substring(line, split+1, line.Length()-split-1);

nit: spaces around operators
applies on more places

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:343
(Diff revision 1)
> -        LOG(("malformed header [%s]: field-name not a token\n", line));
> +        LOG(("malformed header [%s]: field-name not a token\n",
> +            PromiseFlatCString(line).get()));
>          return NS_ERROR_FAILURE;
>      }
>  
> -    *p = 0; // null terminate field-name
> +    // *p = 0; // null terminate field-name

just remove this.

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:356
(Diff revision 1)
>      // skip over whitespace
> -    p = net_FindCharNotInSet(++p, HTTP_LWS);
> +    char *p = net_FindCharNotInSet(
> +        sub2.BeginReading(), sub2.EndReading(), HTTP_LWS);
>  
>      // trim trailing whitespace - bug 86608
> -    char *p2 = net_RFindCharNotInSet(p, HTTP_LWS);
> +    char *p2 = net_RFindCharNotInSet(p, sub2.EndReading(), HTTP_LWS);



::: netwerk/protocol/http/nsHttpHeaderArray.cpp:369
(Diff revision 1)
> +    //     }
> +    // }
> +
> +    // *++p2 = 0; // null terminate header value; if all chars starting at |p|
> +    //            // consisted of LWS, then p2 would have pointed at |p-1|, so
> +    //            // the prefix increment is always valid.

Please remove this comment

::: netwerk/protocol/http/nsHttpRequestHead.cpp:263
(Diff revision 1)
>          char *eof = strchr(buffer, '\r');
>          if (!eof) {
>              break;
>          }
>          *eof = '\0';
> -        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(buffer,
> +        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(nsAutoCString(buffer),

definitely use nsDependentCString, you also know the length of the substring you work here with, nsAutoCString does an unnecessary copy.  or say why you are using it

::: netwerk/protocol/http/nsHttpResponseHead.cpp:299
(Diff revision 1)
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;
> -        ParseHeaderLine_locked(block, false);
> +        ParseHeaderLine_locked(nsDependentCString(block), false);

you know the length of the substring, pass it to the dep string ctor

::: netwerk/protocol/http/nsHttpResponseHead.cpp:335
(Diff revision 1)
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;
> -        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(block, &hdr, &val))) {
> +        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(block), &hdr, &val))) {

I think same applies here
Attachment #8760923 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Comment on attachment 8760923 [details]
> Bug 1277019: change header parsing methods to take nsACString instead of
> char array;
> 
> https://reviewboard.mozilla.org/r/58328/#review56398
> 
> why are the webplatform tests removed?  if those were failing and this is
> fixing them, we have to change the expectation, not remove the failing
> tests.  or am I missing something.

i don't understand what you mean. i only removed a couple of .ini files that were solely used to mark tests as failing. since the tests in question are no longer failing, there is no longer a need for those files.

the rest of your feedback makes sense to me.
(In reply to Honza Bambas (:mayhemer) from comment #7)
> why are the webplatform tests removed?  if those were failing and this is
> fixing them, we have to change the expectation, not remove the failing
> tests.  or am I missing something.

Yeah, like Decky said, these .ini file removals are essentially signaling that the tests fully pass now.

The way that WPT tests work is that the default expectation for each test assertion is that it must pass.  If Gecko fails a specific check, we add a .ini file for the test file and add an expected failure annotation to the .ini file.  Note that there are more than one assertion in each test file, so there may be more than one expected failure in an .ini file (contrary to let's say mochitests).  When you fix one of these failures, the corresponding expected failure needs to be removed as well.  If that's the last expected failure in the .ini file, the .ini file itself must be removed as well.

> is there a test to check that:
> - the original problem is reproducible w/o the patch and irreproducible w/
> the patch

As I explained above, the patch is already doing exactly that!

> - corner cases (zero chars at the begining, end, middle, whitespaces at the
> start, end, only whitespaces, only zero chars)
> 
> ?
> 
> If not, they must be added.  This is an easily exploitable code when wrong,
> so it should be tested well.

Yeah, it's a good idea to add such tests.  I think we can extend the existing checks here...
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/1-2/
Attachment #8760923 - Flags: review- → review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #7)

i updated the commit to include most of the changes you suggested, except for these:

> ::: netwerk/protocol/http/nsHttpRequestHead.cpp:263
> (Diff revision 1)
> >          char *eof = strchr(buffer, '\r');
> >          if (!eof) {
> >              break;
> >          }
> >          *eof = '\0';
> > -        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(buffer,
> > +        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(nsAutoCString(buffer),
> 
> definitely use nsDependentCString, you also know the length of the substring
> you work here with, nsAutoCString does an unnecessary copy.  or say why you
> are using it
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:299
> (Diff revision 1)
> >          p = PL_strstr(block, "\r\n");
> >          if (!p)
> >              return NS_ERROR_UNEXPECTED;
> >  
> >          *p = 0;
> > -        ParseHeaderLine_locked(block, false);
> > +        ParseHeaderLine_locked(nsDependentCString(block), false);
> 
> you know the length of the substring, pass it to the dep string ctor
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:335
> (Diff revision 1)
> >          p = PL_strstr(block, "\r\n");
> >          if (!p)
> >              return NS_ERROR_UNEXPECTED;
> >  
> >          *p = 0;
> > -        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(block, &hdr, &val))) {
> > +        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(block), &hdr, &val))) {
> 
> I think same applies here

i switched to nsDependentCString but i couldn't see how i would know the length of the substrings.
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/2-3/
(In reply to Decky Coss (:deckycoss) from comment #12)
> Comment on attachment 8760923 [details]
> Bug 1277019: change header parsing methods to take nsACString instead of
> char array;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/58328/diff/2-3/

please ignore the commit i made earlier today and look at this one instead. my changes got lost in the earlier commit.
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/3-4/
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

https://reviewboard.mozilla.org/r/58328/#review57630

do you plan to create a test for this particular code (zero chars at the beginning, end, middle, white spaces at the start, end, only white spaces, only zero chars) or is there a test that already exercise this?

::: netwerk/protocol/http/nsHttpRequestHead.cpp:264
(Diff revisions 1 - 4)
>          if (!eof) {
>              break;
>          }
>          *eof = '\0';
> -        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(nsAutoCString(buffer),
> +        if (NS_SUCCEEDED(
> +            nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(buffer),

length here is obviously |eof - buffer|.  you then also don't need to do the *eof = 0 assignment and you may also make the arg be char const *buffer and turn the method such way to a non-destructable one!  and you could then save some string copying?  I hope I'm right...

::: netwerk/protocol/http/nsHttpResponseHead.cpp:299
(Diff revision 4)
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;
> -        ParseHeaderLine_locked(block, false);
> +        ParseHeaderLine_locked(nsDependentCString(block), false);

apparently the length here is known and it is |p - block|

::: netwerk/protocol/http/nsHttpResponseHead.cpp:335
(Diff revision 4)
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;
> -        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(block, &hdr, &val))) {
> +        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(block), &hdr, &val))) {

same here..
Attachment #8760923 - Flags: review?(honzab.moz) → review-
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/4-5/
Attachment #8760923 - Flags: review- → review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #15)
> Comment on attachment 8760923 [details]
> Bug 1277019: change header parsing methods to take nsACString instead of
> char array;
> 
> https://reviewboard.mozilla.org/r/58328/#review57630
> 
> do you plan to create a test for this particular code (zero chars at the
> beginning, end, middle, white spaces at the start, end, only white spaces,
> only zero chars) or is there a test that already exercise this?

my latest commit adds a new web-platform test to check for these edge cases. (there's also an existing test for some of them here: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/headers/headers-normalize.html)

> ::: netwerk/protocol/http/nsHttpRequestHead.cpp:264
> (Diff revisions 1 - 4)
> >          if (!eof) {
> >              break;
> >          }
> >          *eof = '\0';
> > -        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(nsAutoCString(buffer),
> > +        if (NS_SUCCEEDED(
> > +            nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(buffer),
> 
> length here is obviously |eof - buffer|.  you then also don't need to do the
> *eof = 0 assignment and you may also make the arg be char const *buffer and
> turn the method such way to a non-destructable one!  and you could then save
> some string copying?  I hope I'm right...
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:299
> (Diff revision 4)
> >          p = PL_strstr(block, "\r\n");
> >          if (!p)
> >              return NS_ERROR_UNEXPECTED;
> >  
> >          *p = 0;
> > -        ParseHeaderLine_locked(block, false);
> > +        ParseHeaderLine_locked(nsDependentCString(block), false);
> 
> apparently the length here is known and it is |p - block|
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:335
> (Diff revision 4)
> >          p = PL_strstr(block, "\r\n");
> >          if (!p)
> >              return NS_ERROR_UNEXPECTED;
> >  
> >          *p = 0;
> > -        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(block, &hdr, &val))) {
> > +        if (NS_FAILED(nsHttpHeaderArray::ParseHeaderLine(nsDependentCString(block), &hdr, &val))) {
> 
> same here..

done.
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/5-6/
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

https://reviewboard.mozilla.org/r/58328/#review60980

some small details left to tune

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:304
(Diff revisions 4 - 6)
>              continue;
>          }
> -        if (NS_FAILED(visitor->VisitHeader(nsDependentCString(entry.header),
> -                                           entry.value))) {
> -            break;
> +        rv = visitor->VisitHeader(
> +            nsDependentCString(entry.header), entry.value);
> +        if NS_FAILED(rv) {
> +            return rv;

why exactly are you returning the error now?  if you found an issue and this is its fix, you must add a comment.  something tells me this could break stuff unintentionally.

::: netwerk/protocol/http/nsHttpRequestHead.cpp:258
(Diff revisions 4 - 6)
>  {
>      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>      nsHttpAtom hdr;
>      nsAutoCString val;
>      while (buffer) {
> -        char *eof = strchr(buffer, '\r');
> +        char *eof = strchr((char *) buffer, '\r');

I think there is const char* strchr overload.  mean, don't cast.

::: netwerk/protocol/http/nsHttpRequestHead.cpp
(Diff revisions 4 - 6)
>              mHeaders.SetHeaderFromNet(hdr, val, false);
>          }
> -        buffer = eof + 1;
> -        if (*buffer == '\n') {
> -            buffer++;
> -        }

why has this been removed?  honestly, I don't see a way how this loop would ever end.

if you are on linux, please run netwerk/test/unit/test_http2.js test (./mach test netwerk/test/unit/test_http2.js) that should trigger this code.  I can't check this on win.

::: netwerk/protocol/http/nsHttpResponseHead.cpp:298
(Diff revisions 4 - 6)
>  
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;

ParseCachedHead could potentially be turned to a non-destructable method as well (taking char const *block instead).

you don't need the *p = 0 assignment.

::: netwerk/protocol/http/nsHttpResponseHead.cpp:299
(Diff revisions 4 - 6)
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
>          *p = 0;
> -        ParseHeaderLine_locked(nsDependentCString(block), false);
> +        ParseHeaderLine_locked(nsDependentCString(block, p - block), false);

I gave you a slightly bad advice, please use nsDepenedentCSubstring instead.  I always forget that nsDependentCString needs to be properly zero-ended, what is something we don't want here.
Attachment #8760923 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8760923 [details]
> Bug 1277019: change header parsing methods to take nsACString instead of
> char array;
> 
> https://reviewboard.mozilla.org/r/58328/#review60980
> 
> some small details left to tune
> 
> ::: netwerk/protocol/http/nsHttpHeaderArray.cpp:304
> (Diff revisions 4 - 6)
> >              continue;
> >          }
> > -        if (NS_FAILED(visitor->VisitHeader(nsDependentCString(entry.header),
> > -                                           entry.value))) {
> > -            break;
> > +        rv = visitor->VisitHeader(
> > +            nsDependentCString(entry.header), entry.value);
> > +        if NS_FAILED(rv) {
> > +            return rv;
> 
> why exactly are you returning the error now?  if you found an issue and this
> is its fix, you must add a comment.  something tells me this could break
> stuff unintentionally.

that's not part of this patch; it was added in Bug 1279324.

> ::: netwerk/protocol/http/nsHttpRequestHead.cpp:258
> (Diff revisions 4 - 6)
> >  {
> >      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> >      nsHttpAtom hdr;
> >      nsAutoCString val;
> >      while (buffer) {
> > -        char *eof = strchr(buffer, '\r');
> > +        char *eof = strchr((char *) buffer, '\r');
> 
> I think there is const char* strchr overload.  mean, don't cast.
> 
> ::: netwerk/protocol/http/nsHttpRequestHead.cpp
> (Diff revisions 4 - 6)
> >              mHeaders.SetHeaderFromNet(hdr, val, false);
> >          }
> > -        buffer = eof + 1;
> > -        if (*buffer == '\n') {
> > -            buffer++;
> > -        }
> 
> why has this been removed?  honestly, I don't see a way how this loop would
> ever end.
> 
> if you are on linux, please run netwerk/test/unit/test_http2.js test (./mach
> test netwerk/test/unit/test_http2.js) that should trigger this code.  I
> can't check this on win.

my bad, i removed it in error. i'll put it back.
(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8760923 [details]
> Bug 1277019: change header parsing methods to take nsACString instead of
> char array;
> 
> https://reviewboard.mozilla.org/r/58328/#review60980
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:298
> (Diff revisions 4 - 6)
> >  
> >          p = PL_strstr(block, "\r\n");
> >          if (!p)
> >              return NS_ERROR_UNEXPECTED;
> >  
> >          *p = 0;
> 
> ParseCachedHead could potentially be turned to a non-destructable method as
> well (taking char const *block instead).
> 
> you don't need the *p = 0 assignment.

how would ParseStatusLine_locked know the length of the line without setting *p to 0? should i change the signature so that it accepts a length argument?
(In reply to Decky Coss (:deckycoss) from comment #21)
> (In reply to Honza Bambas (:mayhemer) from comment #19)
> > Comment on attachment 8760923 [details]
> > Bug 1277019: change header parsing methods to take nsACString instead of
> > char array;
> > 
> > https://reviewboard.mozilla.org/r/58328/#review60980
> > 
> > ::: netwerk/protocol/http/nsHttpResponseHead.cpp:298
> > (Diff revisions 4 - 6)
> > >  
> > >          p = PL_strstr(block, "\r\n");
> > >          if (!p)
> > >              return NS_ERROR_UNEXPECTED;
> > >  
> > >          *p = 0;
> > 
> > ParseCachedHead could potentially be turned to a non-destructable method as
> > well (taking char const *block instead).
> > 
> > you don't need the *p = 0 assignment.
> 
> how would ParseStatusLine_locked know the length of the line without setting
> *p to 0? should i change the signature so that it accepts a length argument?

It takes the nsDependentCSubstring class that keeps the length inside.  It goes down to nsHttpHeaderArray::ParseHeaderLine that is using methods on that string class object that is limited to the length.  It works for you automatically.  That's the whole trick of the patch, isn't it?
(In reply to Honza Bambas (:mayhemer) from comment #22)
> (In reply to Decky Coss (:deckycoss) from comment #21)
> > (In reply to Honza Bambas (:mayhemer) from comment #19)
> > > Comment on attachment 8760923 [details]
> > > Bug 1277019: change header parsing methods to take nsACString instead of
> > > char array;
> > > 
> > > https://reviewboard.mozilla.org/r/58328/#review60980
> > > 
> > > ::: netwerk/protocol/http/nsHttpResponseHead.cpp:298
> > > (Diff revisions 4 - 6)
> > > >  
> > > >          p = PL_strstr(block, "\r\n");
> > > >          if (!p)
> > > >              return NS_ERROR_UNEXPECTED;
> > > >  
> > > >          *p = 0;
> > > 
> > > ParseCachedHead could potentially be turned to a non-destructable method as
> > > well (taking char const *block instead).
> > > 
> > > you don't need the *p = 0 assignment.
> > 
> > how would ParseStatusLine_locked know the length of the line without setting
> > *p to 0? should i change the signature so that it accepts a length argument?
> 
> It takes the nsDependentCSubstring class that keeps the length inside.  It
> goes down to nsHttpHeaderArray::ParseHeaderLine that is using methods on
> that string class object that is limited to the length.  It works for you
> automatically.  That's the whole trick of the patch, isn't it?

are you thinking of ParseHeaderLine_locked? i never modified ParseStatusLine_locked, so it still only takes a const char*.

i think i could modify it to take nsDependentCSubstring but i'm not sure that doing so is within the scope of this patch.
(In reply to Decky Coss (:deckycoss) from comment #23)
> are you thinking of ParseHeaderLine_locked? i never modified
> ParseStatusLine_locked, so it still only takes a const char*.
> 

I can see in the patch you are changing the signature to:

nsHttpResponseHead::ParseHeaderLine_locked(const nsACString &line, bool originalFromNetHeaders)

The |line| argument encapsulates the substring and length of the substring portion.  It's passed down to nsHttpHeaderArray::ParseHeaderLine(const nsACString& line, ..) - which you also modify - that is calling methods on its |line| argument.  Those work with the substring portion length, so that e.g. line.FindChar(':') will only scan for ':' only within the substring portion you passed to ParseHeaderLine_locked.

> i think i could modify it to take nsDependentCSubstring but i'm not sure
> that doing so is within the scope of this patch.

I think it is, it's not that complicated and will make the patch more complete.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [necko-would-take] → [necko-active]
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/6-7/
Attachment #8760923 - Attachment description: Bug 1277019: change header parsing methods to take nsACString instead of char array; → Bug 1277019: change header and status parsing methods to take nsACString instead of char array;
Attachment #8760923 - Flags: review- → review?(honzab.moz)
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/7-8/
Attachment #8760923 - Attachment description: Bug 1277019: change header and status parsing methods to take nsACString instead of char array; → Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;
https://reviewboard.mozilla.org/r/58328/#review62842

Too much time spent on this patch.  Really, I would love much more seeing this whole code rewritten with mozilla::Tokenizer.  I think the test is insufficient to check all the corners and there is still a work to do to deliver a complete patch.  Sorry.

Ehsan, what do you think?

::: netwerk/protocol/http/nsHttpResponseHead.h:92
(Diff revisions 6 - 8)
>      // They are used when we are reading an entry from the cache.
>      //
>      // To keep proper order of the original headers we MUST call
>      // ParseCachedOriginalHeaders FIRST and then ParseCachedHead.
>      //
>      // block must be null terminated. parsing is destructive.

please update the comment

::: netwerk/protocol/http/nsHttpResponseHead.cpp:551
(Diff revisions 6 - 8)
> +        if (index == -1) {
>              AssignDefaultStatusText();
>          }
>          else
> -            mStatusText = nsDependentCString(++line);
> +            p = start + index + 1;
> +            mStatusText = nsDependentCSubstring(p, end - p);

hmm.. this is a mistake!

there is a very strong rule we have, and it's to surround ALL if bodies with { }.  Here it's well visible why we enforce that.  Hence, here it has to be:

if (index == -1) {
  ...
} else {
  p = ...;
  mStatusText = ...;
}

did your tests capture it?

::: netwerk/protocol/http/nsHttpTransaction.cpp:1478
(Diff revisions 6 - 8)
>              if (!p) {
>                  // Treat any 0.9 style response of a put as a failure.
>                  if (mRequestHead->IsPut())
>                      return NS_ERROR_ABORT;
>  
> -                mResponseHead->ParseStatusLine("");
> +                mResponseHead->ParseStatusLine(nsDependentCString(""));

please use EmptyCString() instead.

::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp:116
(Diff revision 8)
>                  LOG(("got trailer: %s\n", buf));
>                  // allocate a header array for the trailers on demand
>                  if (!mTrailers) {
>                      mTrailers = new nsHttpHeaderArray();
>                  }
> -                mTrailers->ParseHeaderLine(buf);
> +                mTrailers->ParseHeaderLine(nsDependentCString(buf, count));

I think here you also have to use nsDependentCSubstring

::: netwerk/protocol/http/nsHttpHeaderArray.cpp:347
(Diff revision 8)
> -        LOG(("malformed header [%s]: field-name not a token\n", line));
> +        LOG(("malformed header [%s]: field-name not a token\n",
> +            PromiseFlatCString(line).get()));
>          return NS_ERROR_FAILURE;
>      }
>  
> -    *p = 0; // null terminate field-name
> +    // *p = 0; // null terminate field-name

remove this

::: netwerk/protocol/http/nsHttpRequestHead.cpp:258
(Diff revision 8)
>  {
>      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>      nsHttpAtom hdr;
> -    char *val;
> +    nsAutoCString val;
>      while (buffer) {
> -        char *eof = strchr(buffer, '\r');
> +        const char *eof = strchr(buffer, '\r');

how is this method actually protected against embedded nulls?

I don't think the original problem is fixed here.  Your test only checks what is coming from the network, but not what we reload from the cache.  Yes.. there are two different pieces of code for that :(

::: netwerk/protocol/http/nsHttpRequestHead.cpp:263
(Diff revision 8)
> -        char *eof = strchr(buffer, '\r');
> +        const char *eof = strchr(buffer, '\r');
>          if (!eof) {
>              break;
>          }
> -        *eof = '\0';
> -        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(buffer,
> +        if (NS_SUCCEEDED(nsHttpHeaderArray::ParseHeaderLine(
> +            nsDependentCString(buffer, eof - buffer),

nsDependentCSubstring here as well

::: netwerk/protocol/http/nsHttpResponseHead.cpp:293
(Diff revision 8)
>          block = p + 2;
>  
>          if (*block == 0)
>              break;
>  
>          p = PL_strstr(block, "\r\n");

will stop on \0

::: netwerk/protocol/http/nsHttpResponseHead.cpp:328
(Diff revision 8)
>          block = p;
>  
>          if (*block == 0)
>              break;
>  
>          p = PL_strstr(block, "\r\n");

will stop on \0

::: netwerk/protocol/http/nsHttpTransaction.cpp:1382
(Diff revision 8)
>          // XXX this should probably never happen
>          if (mResponseHead->Version() == NS_HTTP_VERSION_0_9)
>              mHaveAllHeaders = true;
>      }
>      else {
>          rv = mResponseHead->ParseHeaderLine(line);

so, why PromiseFlatCString when calling mResponseHead->ParseStatusLine and not when calling mResponseHead->ParseHeaderLine?

According the code it seems like both do linear pointer arithmetic.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp:1218
(Diff revision 8)
>  
>          char tmpChar = *newLine;
>          *newLine = '\0'; // cursor is now null terminated
>  
>          if (mResponseHead) {
>              // ParseHeaderLine is destructive. We create a copy

no longer?
Ehsan, please see comment 27.
Flags: needinfo?(ehsan)
(In reply to Honza Bambas (:mayhemer) from comment #27)
> https://reviewboard.mozilla.org/r/58328/#review62842
> 
> Too much time spent on this patch.  Really, I would love much more seeing
> this whole code rewritten with mozilla::Tokenizer.  I think the test is
> insufficient to check all the corners and there is still a work to do to
> deliver a complete patch.  Sorry.
> 
> Ehsan, what do you think?

Given that Decky has spent quite a bit of time on this patch, I think it's unfair to ask her to rewrite it from scratch at this point.  Honestly I'm disappointed that the scope of this bug was increased in comment 24, I'd appreciate if we don't do that again here.  In my opinion, follow-up bugs need to be filed for things such as rewriting this using Tokenizer.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #29)
> (In reply to Honza Bambas (:mayhemer) from comment #27)
> > https://reviewboard.mozilla.org/r/58328/#review62842
> > 
> > Too much time spent on this patch.  Really, I would love much more seeing
> > this whole code rewritten with mozilla::Tokenizer.  I think the test is
> > insufficient to check all the corners and there is still a work to do to
> > deliver a complete patch.  Sorry.
> > 
> > Ehsan, what do you think?
> 
> Given that Decky has spent quite a bit of time on this patch, I think it's
> unfair to ask her to rewrite it from scratch at this point.  Honestly I'm
> disappointed that the scope of this bug was increased in comment 24, I'd
> appreciate if we don't do that again here.  In my opinion, follow-up bugs
> need to be filed for things such as rewriting this using Tokenizer.

I understand the original goal of this bug was to accept nulls in headers
coming from the network.  The patch allows that with some drawbacks.

I've wrote a simple raw python server to tests this (fixed) patch.

- We accept headers having a null in the value.  
- We still don't accept headers with nulls in the name, it's an illegal char we filter out (this patch seems not to affect that).  
- We can't load responses with nulls in header values from the cache, nsHttpResponseHead::ParseCachedOriginalHeaders will fail because it won't find the "\r\n" string (still uses strstr), that renders the cache entry unusable but also not doomed.  We will always reload from net (not a big deal, but still, points out how this patch is not complete)


There should be just one followup to this bug - rewrite the code properly with a more mature parser than strstr/strchr and using indexes to substrings.


I will r+ the patch, with few last closing comments.
(In reply to Honza Bambas (:mayhemer) from comment #27)


Decky, please IGNORE the following comments in my last review:


> ::: netwerk/protocol/http/nsHttpRequestHead.cpp:258
> (Diff revision 8)
> >  {
> >      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> >      nsHttpAtom hdr;
> > -    char *val;
> > +    nsAutoCString val;
> >      while (buffer) {
> > -        char *eof = strchr(buffer, '\r');
> > +        const char *eof = strchr(buffer, '\r');
> 
> how is this method actually protected against embedded nulls?
> 
> I don't think the original problem is fixed here.  Your test only checks
> what is coming from the network, but not what we reload from the cache. 
> Yes.. there are two different pieces of code for that :(

Let's go with it.

> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:293
> (Diff revision 8)
> >          block = p + 2;
> >  
> >          if (*block == 0)
> >              break;
> >  
> >          p = PL_strstr(block, "\r\n");
> 
> will stop on \0
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp:328
> (Diff revision 8)
> >          block = p;
> >  
> >          if (*block == 0)
> >              break;
> >  
> >          p = PL_strstr(block, "\r\n");
> 
> will stop on \0

Let's live with these two as well.




All the remaining comments must be fixed.
Attachment #8760923 - Flags: review?(honzab.moz) → review+
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

https://reviewboard.mozilla.org/r/58328/#review64452

See comment 31 and comment 27 for what to update on the patch before landing it.
Also, push to try with a full suite test run.  Only a green (ok, "green" ;)) try is a precondition to land this.
Comment on attachment 8760923 [details]
Bug 1277019: change header and status parsing methods to take nsACString instead of char array ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58328/diff/8-9/
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a79fd415865e
Change header and status parsing methods to take nsACString instead of char array. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a79fd415865e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.