Tcl Source Code

View Ticket
Login
Ticket UUID: 3598590
Title: UpdateStringOfList should use TclConvertElement length
Type: Bug Version: None
Submitter: apnadkarni Created on: 2012-12-27 03:51:24
Subsystem: 14. List Object Assigned To: dgp
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2013-11-12 12:39:32
Resolution: Wont Fix Closed By: dkf
    Closed on: 2013-11-12 12:39:32
Description:
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.
User Comments: dgp added on 2012-12-29 12:29:25:
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.

apnadkarni added on 2012-12-28 11:20:47:
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 ?

dgp added on 2012-12-28 04:05:41:
See the creation of the internal routines TclScanElement and
TclConvertElement at

http://core.tcl.tk/tcl/info/37927cede6

which was the final product of work on the bug-3173086 branch.

http://core.tcl.tk/tcl/timeline?t=bug-3173086

apnadkarni added on 2012-12-27 10:53:50:
Comments from thommey on the chat:

09:05]<thommey>somewhere in between 8.5.9 and 8.5.10
[09:06]apnHow 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]kbkapn: 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]apnYep
[09:14]apnWas using TclConvertElement return values before
[09:14]<thommey>so, it's just a coincidence I remember dealing with that particular issue 
[09:15]apnA 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