Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#422 Invalid read in FindHelp() found by valgrind

open
nobody
None
2
2009-06-01
2009-03-04
No

Ethan has pointed out that my first fix to this in 2657599 is not correct (thanks). This one corrects both the left (i.e. the second) border of the copy loop as well as prevents an invalid write behind the end of the allocated area. I have checked the code path and found that keyword is allocated as MAX_LINE_LEN.
This fix is not really a clean solution, but it is better than the current state.

Juergen

Discussion

  • Ethan Merritt
    Ethan Merritt
    2009-03-04

    Sorry to be so picky, but this fix is still not correct.

    It is true that the original buffer was allocated with length MAX_LINE_LEN. But the keyword being queried may in principle start anywhere within this buffer, so we do not know inside the FindHelp() routine how much space remains in the buffer that was passed to us. I think the whole mechanism is flawed. In order for this operation to be safe, the low level routine would have to receive the buffer start, length, and current index rather than, as now, receiving only a pointer to somewhere in the buffer. Better yet, the lower level routine should not mess with the buffer at all. It could instead return a pointer to the full-length keyword and let the calling routine use this to modify its own buffer.

    In practice no legitimate help query will involve passing a nearly-full MAX_LINE_LEN buffer. But if we're going to the trouble of fixing possible buffer overflows, let's do a proper job of it.

     
  • True. I should not meddle with code paths I'm not familiar with.

    I'd propose to either move this item to the Bugs or to close it, then.

    Juergen

     
  • Ethan Merritt
    Ethan Merritt
    2009-06-01

    In preparation for eventual release of gnuplot version 4.4, existing bugs and patchsets are being re-prioritized.

    I have chosen to use the following categories

    9 - should be included in 4.4.rc1 if possible
    7 - looks reasonable to me, but not high priority for 4.4.rc1
    5 - default priority for newly submitted patches
    2 - not under active consideration

    These are my personal ratings. If you want to argue for a different priority on one or more patches - go right ahead.

    Ethan Merritt (sfeam)

     
  • Ethan Merritt
    Ethan Merritt
    2009-06-01

    • priority: 5 --> 2