[webkit-reviews] review denied: [Bug 29674] meta http-equiv Refresh is not honored when 'space' instead of semicolon is used. : [Attachment 40389] patch updated

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 1 11:55:52 PDT 2009


Darin Adler <darin at apple.com> has denied Jungshik Shin <jshin at chromium.org>'s
request for review:
Bug 29674: meta http-equiv Refresh is not honored when 'space' instead of
semicolon is used.
https://bugs.webkit.org/show_bug.cgi?id=29674

Attachment 40389: patch updated
https://bugs.webkit.org/attachment.cgi?id=40389&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (pos < len && refresh[pos] == ' ') {
> +	  skipWhiteSpace(refresh, pos, fromHttpEquivMeta);
> +	  if (refresh[pos] != ',' && refresh[pos] != ';')
> +	      --pos;
> +    }

I believe that after calling skipWhiteSpace, pos may point past the end of the
buffer, so it's unsafe to access it without checking that it's != len. The
return value from skipWhiteSpace will be of help, in fact.

There's no reason to check for the space before calling skipWhiteSpace, since
that function already does so.

I think there are way too few tests here. When we have a parser, we need a test
that covers all the various branches in the parser, and this adds only one
case. We should explore to see if we can find a technique to use where we can
test all the various branches in this parser? It's critical that the parser not
have bugs like buffer overruns, so thorough testing is valuable.

review- because of the buffer overrun


More information about the webkit-reviews mailing list