Bug 78521 - The comment about terminating nulls at the top of CSSParser::lex() is incorrect
Summary: The comment about terminating nulls at the top of CSSParser::lex() is incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Trivial
Assignee: Cris Neckar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 13:21 PST by Cris Neckar
Modified: 2012-02-16 23:48 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.26 KB, patch)
2012-02-14 17:19 PST, Cris Neckar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cris Neckar 2012-02-13 13:21:53 PST
We should fix the comment 

// The input buffer is terminated by two \0, so
// it is safe to read two characters ahead anytime

at the top of CSSParser::lex(). It is wrong and suggests that there are 2 characters of nulls. This should probably say something like

// The input buffer is terminated by a \0 character, so
// it is safe to read one character ahead of a known non-null
Comment 1 Cris Neckar 2012-02-14 17:19:34 PST
Created attachment 127087 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-16 13:36:24 PST
Difficult for me to review this, because I don't know enough about lex().   Who creates this buffer?  Bison?  Can we point to the bison docs where it describes the buffer it creates?
Comment 3 Cris Neckar 2012-02-16 14:23:46 PST
(In reply to comment #2)
> Difficult for me to review this, because I don't know enough about lex().   Who creates this buffer?  Bison?  Can we point to the bison docs where it describes the buffer it creates?

There is no real immediacy here as it is just a comment change. Zoltan can review this when he has time.
Comment 4 David Barr 2012-02-16 16:31:08 PST
I think the comment is right but the code is wrong.
CSSParser::lex depends on this invariant to detect the end of the buffer.
Comment 5 Cris Neckar 2012-02-16 16:40:19 PST
The one case where it depended on it was just changed. I'm not sure whether it was originally supposed to be one or 2 nulls but at this point only one is written and lex() only relies on there being one.
Comment 6 David Barr 2012-02-16 16:56:47 PST
Thanks for the clarification, I see that dependence on double-null was removed recently:
http://trac.webkit.org/changeset/107369
Comment 7 Zoltan Herczeg 2012-02-16 22:55:32 PST
Comment on attachment 127087 [details]
Patch

The double NULL originated from flex. The last comment here explains it:

http://stackoverflow.com/questions/1909166/how-to-parse-from-a-string-rather-than-a-file

When I started working on the lexer, I kept this behaviour first. Later I realized I didn't need double NULL terminator, but the comment was remained and the comment parsing was wrong.
Comment 8 WebKit Review Bot 2012-02-16 23:48:14 PST
Comment on attachment 127087 [details]
Patch

Clearing flags on attachment: 127087

Committed r108043: <http://trac.webkit.org/changeset/108043>
Comment 9 WebKit Review Bot 2012-02-16 23:48:20 PST
All reviewed patches have been landed.  Closing bug.