Bug 176357 – Missing break in URLParser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tomas Popela]

Found by Coverity scan:

1. webkitgtk-2.16.6/Source/WebCore/platform/URLParser.cpp:1233: value_overwrite: Overwriting previous write to "state" with value "void WebCore::URLParser::parse(unsigned short const *, unsigned int, WebCore::URL const &, WebCore::TextEncoding const &)::State::Scheme (instance 69995)".
2. webkitgtk-2.16.6/Source/WebCore/platform/URLParser.cpp:1230: assigned_value: Assigning value "void WebCore::URLParser::parse(unsigned short const *, unsigned int, WebCore::URL const &, WebCore::TextEncoding const &)::State::NoScheme (instance 69996)" to "state" here, but that stored value is overwritten before it can be used.
#  1228|                   if (c.atEnd()) {
#  1229|                       m_asciiBuffer.clear();
#  1230|->                     state = State::NoScheme;
#  1231|                       c = beginAfterControlAndSpace;
#  1232|                   }

[Tomas Popela]

Created attachment 319889 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 319889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319889&action=review

> Source/WebCore/ChangeLog:9
> +        Add a missing break so the currently set state is not overwritten
> +        after the block. Found by Coverity scan.

Does this change observable behavior? If so, please add a test.

[Darin Adler]

Comment on attachment 319889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319889&action=review

>> Source/WebCore/ChangeLog:9
>> +        after the block. Found by Coverity scan.
> 
> Does this change observable behavior? If so, please add a test.

Yes, it’s a great catch, but we do want a regression test to show what we were doing wrong!

[Tomas Popela]

(In reply to Darin Adler from comment #3)
> Comment on attachment 319889 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319889&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +        after the block. Found by Coverity scan.
> > 
> > Does this change observable behavior? If so, please add a test.
> 
> Yes, it’s a great catch, but we do want a regression test to show what we
> were doing wrong!

I don't think we need test case if I got the context right:

From looking at the code the sequence to get to the part where the 'break' is missing we have to process the input that contains one character. We will start with state SchemeStart (log this state), process the character (that has to be ASCII lowercase alpha) and append to the buffer. Now we will move to the next character and as there is none (c.atEnd() is true) we will move to the branch with missing 'break'. The state is set to NoScheme, the input restarted, buffer cleared and the state is again changed, but to Scheme (due to missing 'break'). We continue in the loop (!c.atEnd()), now with the Scheme state. The state is logged and the first condition for Scheme case is isValidSchemeCharacter(). We know that the input contains ASCII lowercase alpha and that means that it is also a valid scheme character. The character is appended to the buffer and we move to the next character in the input, but as there is none then the state is set to NoScheme, the input restarted, buffer cleared. We again continue in the loop, now with the state NoScheme. So we got to the same point where we would end with the 'break' added in this patch. So the main difference is that without the 'break' there is an extra log message mentioning Scheme state. I should note that the url parser debugging is disabled by default, so in the default configuration there is no difference while running URLParser with or without this patch.

[Darin Adler]

Comment on attachment 319889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319889&action=review

>>>> Source/WebCore/ChangeLog:9
>>>> +        after the block. Found by Coverity scan.
>>> 
>>> Does this change observable behavior? If so, please add a test.
>> 
>> Yes, it’s a great catch, but we do want a regression test to show what we were doing wrong!
> 
> I don't think we need test case if I got the context right:
> 
> From looking at the code the sequence to get to the part where the 'break' is missing we have to process the input that contains one character. We will start with state SchemeStart (log this state), process the character (that has to be ASCII lowercase alpha) and append to the buffer. Now we will move to the next character and as there is none (c.atEnd() is true) we will move to the branch with missing 'break'. The state is set to NoScheme, the input restarted, buffer cleared and the state is again changed, but to Scheme (due to missing 'break'). We continue in the loop (!c.atEnd()), now with the Scheme state. The state is logged and the first condition for Scheme case is isValidSchemeCharacter(). We know that the input contains ASCII lowercase alpha and that means that it is also a valid scheme character. The character is appended to the buffer and we move to the next character in the input, but as there is none then the state is set to NoScheme, the input restarted, buffer cleared. We again continue in the loop, now with the state NoScheme. So we got to the same point where we would end with the 'break' added in this patch. So the main difference is that without the 'break' there is an extra log message mentioning Scheme state. I should note that the url parser debugging is disabled by default, so in the default configuration there is no difference while running URLParser with or without this patch.

Yes, makes sense. This mistake did not result in incorrect behavior, just a *tiny* bit less efficient.

[WebKit Commit Bot]

Comment on attachment 319889 [details]
Patch

Clearing flags on attachment: 319889

Committed r221677: <http://trac.webkit.org/changeset/221677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693623>