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

Close

#5154 UpdateStringOfList should use TclConvertElement length

pending-wont-fix
Don Porter
5
2012-12-29
2012-12-27
No

tclListObj.c:UpdateStringOfList (which is what I'm cloning for an extension), loops TclScanElement to estimate size of buffer. It then sets Tcl_Obj.length as this estimate. As per source comments for TclScanElement this number is a *over*estimate and it is the lengths returned from TclConverElement that should be used. The only way it works as is if TclScanElement always returns a exact count, never an over estimate I can't actually get it to return an overestimate (yet) Either TclScanElement docs are wrong or the code needs to be fixed to use the lengths returned by TclConvertElement Yes, highly unlikely USOL does not work! But I think it is more likely the estimate is precise than the overallocation being harmless else you would get trailing garbage in some cases, I think (both the length field and the terminating null point to the end of allocated space) Other possibility is that the overestimate is some really arcane case that is not in the tests and will rarely (never?) occur in real life It appears as though the extra space happens (afaict) only when the CONVERT_ANY flag is passed in to TclScanElement.

Since UpdateStringOfList does not pass this flag, it appears to be safe. However, to keep to the documented TclScanElement interface and guard against future changes, I think it should use the lengths returned by TclConvertElement instead.

Discussion

  • Comments from thommey on the chat:

    09:05] <thommey> somewhere in between 8.5.9 and 8.5.10
    [09:06] apn How do you tell that so quickly?
    [09:06] <thommey> I remember that eggdrop relied on it being an overestimate
    [09:06] <thommey> and segfaulted after that was changed
    [09:06] kbk apn: When in doubt, log it. We can always close it.
    [09:07] <thommey> (because eggdrop didn't account for \0 on its own, it relied on it being an overestimate to account for that)
    [09:10] <thommey> http://core.tcl.tk/tcl/info/a142b240b3d0fb1e0757f103fd6129e80d93f0b4 is a likely candidate
    [09:11] <jfe> exits, stage left!
    [09:13] apn Yep
    [09:14] apn Was using TclConvertElement return values before
    [09:14] <thommey> so, it's just a coincidence I remember dealing with that particular issue
    [09:15] apn A lot of changes though, so not clear whether the change to using TclScanElement values for an oversight, or coincides with TclScanElement being modified to be exact (without docs being updated)
    [09:15] <thommey> http://core.tcl.tk/tcl/info/7720fbb825ec366737ab7e8c577ea25315c0a00e is the original change
    [09:16] <thommey> (just walked through my logfiles, I did mention my issue here)
    [09:18] <thommey> hm no those are seperate checkins, no idea how they're related, the latter one is the one I dug out the last time

     
  • Don Porter
    Don Porter
    2012-12-27

    • assigned_to: nijtmans --> dgp
     
  • So the implication is that TclScanElement guarantees exactness except when the CONVERT_ANY flag is set and since UpdateStringOfList does not set this flag, it is assured to get exact counts back. Right ?

     
  • Don Porter
    Don Porter
    2012-12-29

    CONVERT_ANY is the way the new internals
    preserve compatibility of the public Tcl_ScanElement()
    routine. When that flag is set, an overestimate is performed.
    This is described in comments near the top of tclUtil.c where
    flag values are defined.

    Otherwise, TclScanElement expects callers to pass in the
    flags it needs to calculate the exact space requirements, and
    then does so.

    UpdateStringOfList is not broken, but if someone wants to change
    it to an equivalent that would be correct also in the event
    TclScanElement stops being precise, I don't object. Improved
    comments are fine with me too.

     
  • Don Porter
    Don Porter
    2012-12-29

    • status: open --> pending-wont-fix