176357 2017-09-05 03:01:21 -0700 Missing break in URLParser 2017-09-27 12:37:23 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tpopela tpopela achristensen ap commit-queue darin webkit-bug-importer oldest_to_newest 1345390 0 tpopela 2017-09-05 03:01:21 -0700 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| } 1345391 1 319889 tpopela 2017-09-05 03:04:10 -0700 Created attachment 319889 Patch 1345770 2 319889 ap 2017-09-05 19:49:16 -0700 Comment on attachment 319889 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. 1345794 3 319889 darin 2017-09-05 20:56:46 -0700 Comment on attachment 319889 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! 1345894 4 tpopela 2017-09-06 05:46:41 -0700 (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. 1345946 5 319889 darin 2017-09-06 09:08:55 -0700 Comment on attachment 319889 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. 1345962 6 319889 commit-queue 2017-09-06 09:38:00 -0700 Comment on attachment 319889 Patch Clearing flags on attachment: 319889 Committed r221677: <http://trac.webkit.org/changeset/221677> 1345963 7 commit-queue 2017-09-06 09:38:02 -0700 All reviewed patches have been landed. Closing bug. 1353536 8 webkit-bug-importer 2017-09-27 12:37:23 -0700 <rdar://problem/34693623> 319889 2017-09-05 03:04:10 -0700 2017-09-06 09:38:00 -0700 Patch bug-176357-20170905120409.patch text/plain 1409 tpopela U3VidmVyc2lvbiBSZXZpc2lvbjogMjIxNjEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYzk5NmE1NWQyMjllYTI1 MTA3ZmQ4MThjOGY2NGFlMWNkODdlN2QzYS4uNGYxYThmNDFhMGJhNTQyNDUyNTFkZDQ1ZmFhMDBl NjI1MzVkZDYwMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE3LTA5LTA1ICBUb21h cyBQb3BlbGEgIDx0cG9wZWxhQHJlZGhhdC5jb20+CisKKyAgICAgICAgTWlzc2luZyBicmVhayBp biBVUkxQYXJzZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE3NjM1NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEFkZCBhIG1pc3NpbmcgYnJlYWsgc28gdGhlIGN1cnJlbnRseSBzZXQgc3RhdGUgaXMgbm90 IG92ZXJ3cml0dGVuCisgICAgICAgIGFmdGVyIHRoZSBibG9jay4gRm91bmQgYnkgQ292ZXJpdHkg c2Nhbi4KKworICAgICAgICAqIHBsYXRmb3JtL1VSTFBhcnNlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpVUkxQYXJzZXI6OnBhcnNlKToKKwogMjAxNy0wOS0wNSAgWW9zaGlha2kgSml0c3VrYXdh ICA8WW9zaGlha2kuSml0c3VrYXdhQHNvbnkuY29tPgogCiAgICAgICAgIFtXaW5dIEZpeCB0aGUg d2luY2Fpcm8gYnVpbGQgYWZ0ZXIgcjIyMTU1OCBhbmQgcjIyMTU4MwpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vVVJMUGFyc2VyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL1VSTFBhcnNlci5jcHAKaW5kZXggMTU0NTEyYzA1MjdjODI2NWVhZDNlZGI4MjU0MzI5MzMx Mzc3MDBkMy4uOTZmZWI4NWZiZWZkMWU4ZjY4NzE3YWY3YmQ1NjAxY2I1NzgxOGIzZCAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vVVJMUGFyc2VyLmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9VUkxQYXJzZXIuY3BwCkBAIC0xMjUzLDYgKzEyNTMsNyBAQCB2b2lk IFVSTFBhcnNlcjo6cGFyc2UoY29uc3QgQ2hhcmFjdGVyVHlwZSogaW5wdXQsIGNvbnN0IHVuc2ln bmVkIGxlbmd0aCwgY29uc3QgVQogICAgICAgICAgICAgICAgICAgICBtX2FzY2lpQnVmZmVyLmNs ZWFyKCk7CiAgICAgICAgICAgICAgICAgICAgIHN0YXRlID0gU3RhdGU6Ok5vU2NoZW1lOwogICAg ICAgICAgICAgICAgICAgICBjID0gYmVnaW5BZnRlckNvbnRyb2xBbmRTcGFjZTsKKyAgICAgICAg ICAgICAgICAgICAgYnJlYWs7CiAgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgICAgIHN0 YXRlID0gU3RhdGU6OlNjaGVtZTsKICAgICAgICAgICAgIH0gZWxzZQo=