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| }
Created attachment 319889 [details] Patch
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.
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!
(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.
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.
Comment on attachment 319889 [details] Patch Clearing flags on attachment: 319889 Committed r221677: <http://trac.webkit.org/changeset/221677>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693623>