Fixed stack overflow in CPPextension function. An arbitrary size buffer was b...

Fixed stack overflow in CPPextension function. An arbitrary size buffer was being used for extension name. Changed it to use MAX_SYMBOL_NAME_LEN.
- Also formalized the values for MAX_SYMBOL_NAME_LEN and MAX_STRING_LEN. They were being used as if there was a confusion whether it included the NULL terminator or not.
- Fixed some minor issues with code releated to the usage of MAX_SYMBOL_NAME_LEN and MAX_STRING_LEN.
BUG=59625 (crbug.com)

[kbr1, 2010-10-19 22:48:31]

LGTM with one comment.

http://codereview.appspot.com/2585042/diff/1/src/compiler/preprocessor/cpp.c
File src/compiler/preprocessor/cpp.c (right):

http://codereview.appspot.com/2585042/diff/1/src/compiler/preprocessor/cpp.c#...
src/compiler/preprocessor/cpp.c:685: strcpy(extensionName, GetAtomString(atable,
yylvalpp->sc_ident));
Would it not be safer to use strncpy?

[Alok Priyadarshi, 2010-10-20 17:26:32]

Daniel?

http://codereview.appspot.com/2585042/diff/1/src/compiler/preprocessor/cpp.c
File src/compiler/preprocessor/cpp.c (right):

http://codereview.appspot.com/2585042/diff/1/src/compiler/preprocessor/cpp.c#...
src/compiler/preprocessor/cpp.c:685: strcpy(extensionName, GetAtomString(atable,
yylvalpp->sc_ident));
On 2010/10/19 22:48:31, kbr1 wrote:
> Would it not be safer to use strncpy?

Not necessary because MAX_SYMBOL_NAME_LEN is consistently used everywhere, but
yes safer. DONE.

[dgkoch, 2010-10-20 19:20:14]

One minor formatting thing and a question.

Looks good otherwise.

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
File src/compiler/preprocessor/scanner.c (right):

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
src/compiler/preprocessor/scanner.c:352: symbol_name[len++] = ch;
rogue tabs on the end of this line?

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
src/compiler/preprocessor/scanner.c:359: assert(len <= MAX_SYMBOL_NAME_LEN);
This assertion will do nothing in release mode.  Does that mean we should still
have the former check here, or is overflowing symbol_name prevented somewhere
before this function?

[Alok Priyadarshi, 2010-10-20 19:41:56]

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
File src/compiler/preprocessor/scanner.c (right):

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
src/compiler/preprocessor/scanner.c:352: symbol_name[len++] = ch;
On 2010/10/20 19:20:14, dgkoch wrote:
> rogue tabs on the end of this line?

Done.

http://codereview.appspot.com/2585042/diff/5001/src/compiler/preprocessor/sca...
src/compiler/preprocessor/scanner.c:359: assert(len <= MAX_SYMBOL_NAME_LEN);
On 2010/10/20 19:20:14, dgkoch wrote:
> This assertion will do nothing in release mode.  Does that mean we should
still
> have the former check here, or is overflowing symbol_name prevented somewhere
> before this function?

This assertion is correct. len is only incremented if "len <
MAX_SYMBOL_NAME_LEN". You can see this in the line that had rogue tabs.

BUT I do not see a similar guard in a few switch-cases in this function. Mozilla
guys have reported an overflow bug as well so I will revisit this function again
to fix those cases.

[dgkoch, 2010-10-20 20:01:37]

... right I need to increase my mental patch review line-rentention policy...

http://codereview.appspot.com/2585042/diff/11001/src/compiler/preprocessor/sc...
File src/compiler/preprocessor/scanner.c (right):

http://codereview.appspot.com/2585042/diff/11001/src/compiler/preprocessor/sc...
src/compiler/preprocessor/scanner.c:352: symbol_name[len++] = ch;
you got rid of all the other tabs, but these tabs are still on the end of this
line...

[Alok Priyadarshi, 2010-10-20 20:09:59]

http://codereview.appspot.com/2585042/diff/11001/src/compiler/preprocessor/sc...
File src/compiler/preprocessor/scanner.c (right):

http://codereview.appspot.com/2585042/diff/11001/src/compiler/preprocessor/sc...
src/compiler/preprocessor/scanner.c:352: symbol_name[len++] = ch;
On 2010/10/20 20:01:38, dgkoch wrote:
> you got rid of all the other tabs, but these tabs are still on the end of this
> line...

urgh...may be "untabify all" does not mean what I think it does.. DONE.

[dgkoch, 2010-10-20 20:35:26]

thanks. LGTM now