Menu

#216 parser breaks on NUL before preprocessor command line

None
open
5
2020-01-29
2007-06-10
J Phelps
No

CScope dumped core on me when it encountered a binary file with a .C extension (caused by code in fscanner.l that didn't check to see if strpbrk() returned NULL).

While debugging, I found an additional memory bug that has gone unnoticed (a function looks past the end of a local buffer when checking a file's extension).

Both of those bugs are fixed in the patch.

Discussion

  • J Phelps

    J Phelps - 2007-06-10

    Patch to fix bugs.

     
  • J Phelps

    J Phelps - 2007-06-10

    Logged In: YES
    user_id=220197
    Originator: YES

    File Added: cscope.diff.bz2

     
  • Neil Horman

    Neil Horman - 2007-06-10

    Logged In: YES
    user_id=827328
    Originator: NO

    I'm not sure why you added the filelist=11027 clause in the build function, and it doesn't appear in your changelogs, so I'm inclined to remove that. Also, I'm not overly confident in cscopes ability to parse dependency files, so I'm not sure I'm big on the addition of the 'd' clause. Beyond that though, the patch looks good. I'll apply it shortly. Thanks!

     
  • J Phelps

    J Phelps - 2007-06-10

    Logged In: YES
    user_id=220197
    Originator: YES

    I'm not aware of the "filelist=11027" clause. Navigating to the "build" function with cscope doesn't show me that clause, and it doesn't appear in the output of 'grep -rl filelist=11027 .' on my copy of the source.

    The 'd' clause is for the D programming language, which has C-like syntax. It has nothing to do with dependency files.

     
  • Hans-Bernhard Broeker

    Logged In: YES
    user_id=27517
    Originator: NO

    I reject the 'd' change --- D is just way too rare to warrant the unconditional assumption that every *.d file must be a valid C-like source file. We don't do that for *.java, and we shouldn't do it for D. People who really need that will have to go via filelists (cscope.files).

    On a more principal level, I dislike patches containing such unrelated changes, especially if they're not even mentioned in their description here in the tracker. This almost makes it look like you tried to smuggle this change into the code base.

    As to the principal patch about strpbrk, I'm reasonably sure that cannot be useful. This strpbrk() call operates on a buffer that (f)lex already matched to a pattern containing either a " or a <. There's no way strpbrk(s, "\"<") can return NULL on such input.

    The patch on issrcfile() is good. I'll check it in ASAP.

     
  • J Phelps

    J Phelps - 2007-06-11

    Logged In: YES
    user_id=220197
    Originator: YES

    The strpbrk() call that you say cannot return NULL _did_ return NULL, which was why I got a SIGSEGV while using Cscope. It was that SIGSEGV which caused me to write the patch in the first place. The signal was received somewhere in the incfile() function, which had been passed the very NULL pointer that you're telling me strpbrk() can't return.

     
  • J Phelps

    J Phelps - 2007-06-11

    A C file that Cscope cannot parse without dumping core.

     
  • J Phelps

    J Phelps - 2007-06-11

    Logged In: YES
    user_id=220197
    Originator: YES

    Further investigating the bug (since you don't believe it exists), I found that an #include directive that is preceded by a NUL will match Flex's expression, with the NUL being the first character. The strpbrk() then returns NULL (since "\0#include <foobar.h>" is an empty C string), causing a crash, unless of course your version of Cscope is checking the return value of strpbrk().

    I'm even going to the length of attaching an example .c file that will crash Cscope.
    File Added: break-cscope.c.gz

     
  • Hans-Bernhard Broeker

    • summary: Memory bugs (Patch attached) --> parser breaks on NUL before preprocessor command line
    • Group: -->
     

Log in to post a comment.