#4834 Valgrind finds: invalid read in TclMaxListLength()

closed-fixed
5
2011-06-01
2011-05-31
gustafn
No

When pkg-requiring Tk in tcl8.5.10rc, i get an invalid read in TclMaxListLength() (see below).
OS: Mac OS X, 64bit.

% valgrind --dsymutil=yes /usr/local/src/tcl8.5.10/unix/tclsh
==15297== Memcheck, a memory error detector
==15297== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==15297== Using Valgrind-3.6.1 and LibVEX; rerun with -h for copyright info
==15297== Command: /usr/local/src/tcl8.5.10/unix/tclsh
==15297==

% package req Tk

==15297== Invalid read of size 1
==15297== at 0x10011D958: TclMaxListLength (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10011DECF: Tcl_SplitList (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x100100A7A: TclCreateProc (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1001002B5: Tcl_ProcObjCmd (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003AB86: TclEvalObjvInternal (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003BFAB: TclEvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003B417: Tcl_EvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1000D7610: Tcl_FSEvalFileEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1000547D7: Tcl_SourceObjCmd (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003AB86: TclEvalObjvInternal (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003B25A: Tcl_EvalObjv (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003CC31: TclEvalObjEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== Address 0x100e2d5b4 is 0 bytes after a block of size 20 alloc'd
==15297== at 0x1000111FF: malloc (vg_replace_malloc.c:236)
==15297== by 0x100036185: TclpAlloc (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x100042A49: Tcl_Alloc (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10010A712: Tcl_NewStringObj (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1000F4C45: TclSubstTokens (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003BA72: TclEvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003B417: Tcl_EvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1000D7610: Tcl_FSEvalFileEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x1000547D7: Tcl_SourceObjCmd (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003AB86: TclEvalObjvInternal (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003B25A: Tcl_EvalObjv (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297== by 0x10003CC31: TclEvalObjEx (in /usr/local/ns/lib/libtcl8.5.dylib)
==15297==

Discussion

  • Donal K. Fellows

    Is it possible to duplicate this with a symbols-enabled build? The trace is confusing, and it would really help to know where in TclMaxListLength is the problem that valgrind thinks exists; it's not a long function, but it is enough complex to be tricky.

    This code appears to be identical in 8.6

     
  • gustafn

    gustafn - 2011-06-01

    hmm, compiled with --enable-symbols, using just -g, but valgind does not tell me more. The problem should be easy to reproduce. Here is some more debugging insight. : The crash happens in the \0 comparison

    while (numBytes) {
    fprintf(stderr, " check %p numBytes %d\n", bytes, numBytes);
    if ((numBytes == -1) && (*bytes == '\0')) {

    bytes 0x100debc50
    check 0x100debc50 numBytes -1
    SpaceProc
    SpaceRun
    ....
    SpaceProc
    check 0x100debc61 numBytes -1
    SpaceProc
    check 0x100debc62 numBytes -1
    SpaceProc
    SpaceRun
    check 0x100debc64 numBytes -1
    ==64854== Invalid read of size 1
    ==64854== at 0x10011D85D: TclMaxListLength (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10011DE42: Tcl_SplitList (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x100100916: TclCreateProc (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x100100151: Tcl_ProcObjCmd (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003AA22: TclEvalObjvInternal (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003BE47: TclEvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003B2B3: Tcl_EvalEx (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x1000D74AC: Tcl_FSEvalFileEx (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x100054673: Tcl_SourceObjCmd (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003AA22: TclEvalObjvInternal (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003B0F6: Tcl_EvalObjv (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== by 0x10003CACD: TclEvalObjEx (in /usr/local/ns/lib/libtcl8.5.dylib)
    ==64854== Address 0x100debc64 is 0 bytes after a block of size 20 alloc'd
    ==64854== at 0x1000111FF: malloc (vg_replace_malloc.c:236)

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    Tried this on a 64-bit Ubuntu, and got the same! But only
    with core-8.5-branch, not with trunk.

    Switching Tcl to tag core-8.5.9, and the invalid read disappears,
    so this suggests that the problem is introduced somewhere
    in the Tcl 8.5.10rc history, it looks like it is not Tk related.

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    Stacktrace with more info:

    $ valgrind --dsymutil=yes ./tclsh8.5
    ==17892== Memcheck, a memory error detector
    ==17892== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
    ==17892== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
    ==17892== Command: /usr/bin/tclsh8.5
    ==17892==
    % package require Tk
    ==17892== Conditional jump or move depends on uninitialised value(s)
    ==17892== at 0x4F3DF31: TclMaxListLength (tclUtil.c:130)
    ==17892== by 0x4F3E4C2: Tcl_SplitList (tclUtil.c:491)
    ==17892== by 0x4E9BC33: TclCompileSwitchCmd (tclCompCmds.c:4070)
    ==17892== by 0x4EB0AA0: TclCompileScript (tclCompile.c:1440)
    ==17892== by 0x4EAF2AF: TclSetByteCodeFromAny (tclCompile.c:531)
    ==17892== by 0x4EAF460: SetByteCodeFromAny (tclCompile.c:623)
    ==17892== by 0x4F251CD: ProcCompileProc (tclProc.c:2057)
    ==17892== by 0x4F24A1B: PushProcCallFrame (tclProc.c:1589)
    ==17892== by 0x4F24AC4: TclObjInterpProc (tclProc.c:1649)
    ==17892== by 0x4E6480D: TclEvalObjvInternal (tclBasic.c:3700)
    ==17892== by 0x4E64E7A: Tcl_EvalObjv (tclBasic.c:3898)
    ==17892== by 0x4E66936: TclEvalObjEx (tclBasic.c:5122)
    ==17892==
    ==17892== Invalid read of size 1
    ==17892== at 0x4F3DF2C: TclMaxListLength (tclUtil.c:130)
    ==17892== by 0x4F3E4C2: Tcl_SplitList (tclUtil.c:491)
    ==17892== by 0x4F22FB5: TclCreateProc (tclProc.c:472)
    ==17892== by 0x4F228F1: Tcl_ProcObjCmd (tclProc.c:164)
    ==17892== by 0x4E6480D: TclEvalObjvInternal (tclBasic.c:3700)
    ==17892== by 0x4E65C49: TclEvalEx (tclBasic.c:4399)
    ==17892== by 0x4E6504E: Tcl_EvalEx (tclBasic.c:4056)
    ==17892== by 0x4EFC053: Tcl_FSEvalFileEx (tclIOUtil.c:1807)
    ==17892== by 0x4E7D4FB: Tcl_SourceObjCmd (tclCmdMZ.c:962)
    ==17892== by 0x4E6480D: TclEvalObjvInternal (tclBasic.c:3700)
    ==17892== by 0x4E64E7A: Tcl_EvalObjv (tclBasic.c:3898)
    ==17892== by 0x4E66936: TclEvalObjEx (tclBasic.c:5122)
    ==17892== Address 0x5bcbf04 is 0 bytes after a block of size 20 alloc'd
    ==17892== at 0x4C274A8: malloc (vg_replace_malloc.c:236)
    ==17892== by 0x4E5FFB1: TclpAlloc (tclAlloc.c:706)
    ==17892== by 0x4E6C3CC: Tcl_Alloc (tclCkalloc.c:1056)
    ==17892== by 0x4F2BF58: Tcl_NewStringObj (tclStringObj.c:269)
    ==17892== by 0x4F17C06: TclSubstTokens (tclParse.c:2381)
    ==17892== by 0x4E65707: TclEvalEx (tclBasic.c:4287)
    ==17892== by 0x4E6504E: Tcl_EvalEx (tclBasic.c:4056)
    ==17892== by 0x4EFC053: Tcl_FSEvalFileEx (tclIOUtil.c:1807)
    ==17892== by 0x4E7D4FB: Tcl_SourceObjCmd (tclCmdMZ.c:962)
    ==17892== by 0x4E6480D: TclEvalObjvInternal (tclBasic.c:3700)
    ==17892== by 0x4E64E7A: Tcl_EvalObjv (tclBasic.c:3898)
    ==17892== by 0x4E66936: TclEvalObjEx (tclBasic.c:5122)
    ==17892==

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    Found it! TclMaxListLength() didn't handle the
    situation that a space is followed by a null
    character.

    Fixed in core-8-5-branch and trunk.

    dkf or dgp, please evaluate! Is
    TclIsSpaceProc('\0') expected to
    return true or false? It might
    be easier to adapt TclIsSpaceProc(),
    and leave TclMaxListLength() as is.

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01
    • status: open --> pending-fixed
     
  • Donal K. Fellows

    It's used in a bunch of other places too, and appears to be a kind of attempt to make Tcl a bit more isolated from isspace().

     
  • Don Porter

    Don Porter - 2011-06-01

    TclIsSpaceProc('\0') ought to be returning 0. Assuming
    that's working, the first line of the patch isn't needed, right?

     
  • Don Porter

    Don Porter - 2011-06-01

    I suspect the second line of the patch is also
    mistaken, since that will abort processing on
    a NULL, whether or not numBytes has been exhausted.
    The intent was to continue tolerating embedded NULs.

     
  • Don Porter

    Don Porter - 2011-06-01

    Revised fix committed to core-8-5-branch.
    Please give it a test.

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    Tested, and it works! DGP, you are fully right about
    tolerating embedded NUL's. TclIsSpaceProc('\0')
    is expected to return 1, because isspace('\0')
    returns != 0 as well! So this means your
    modification and your remark is correct.

    Thanks!

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    Please merge to trunk, then this issue can be closed.

     
  • Jan Nijtmans

    Jan Nijtmans - 2011-06-01

    >olerating embedded NUL's. TclIsSpaceProc('\0')
    >is expected to return 1, because isspace('\0')
    >returns != 0 as well! So this means your
    This is not correct! null is not a space
    character, so those functions are supposed
    to return 0, as they do!

     
  • Donal K. Fellows

    NUL is of UNICODE class Cc (a control character) and not WS (a whitespace character) therefore isspace(0) should return 0; if your platform does otherwise, report a bug (but not to us; we don't maintain anyone's libc).

     
  • Donal K. Fellows

    Ugh, mixed up my reading of the table. Still doesn't change the fact that NUL is Cc and not Z*

     
  • Don Porter

    Don Porter - 2011-06-01

    I think we're all in violent agreement now.

    I looked into making a test case. All that should
    be needed to demo the former bug is to pass
    any string with trailing whitespace to Tcl_SplitList().

    A few difficulties with that. First, even when we take
    the buggy path, unless we have a watchdog like valgrind
    keeping an eye on things, we won't necessarily get a failure
    reading past the buffer. Second, we don't currently have
    any test command devoted to testing Tcl_SplitList(). There
    are some Tcl commands (and more Tk commands)
    that use Tcl_SplitList(), but none
    that I would recommend continue to do so for any reason
    other than inertia. All the calls I examined would be better
    off converted to using various Tcl_ListObj*() calls instead.
    Don't want to build a test on a transient implementation detail.

    So the clear path to making a test would be to create a testing
    command for Tcl_SplitList(), but that's too much effort (or I'm
    too lazy) to bother for this. I'm sure I'll regret that later.

     
  • Don Porter

    Don Porter - 2011-06-01
    • status: pending-fixed --> closed-fixed
     
  • Don Porter

    Don Porter - 2011-06-01

    fix merged to trunk.
    Thanks for the help reporting and locating it!

     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks