Tcl Source Code

Kindly reported and analyzed by Roman Puls on tcl-core. The cleanup of the returned-extra-args array involves the following sequence:

   ckfree(leftovers);
   leftovers[nrem] = NULL;
 
This is just plain wrong, and has been so since the first implementation (TIP 265).
The purpose of this ckfree (at this place, where nrem might be nonzero) is unclear to me.
Assigning to the initial committer.

allow_comments - 1

Tests added. Everything merged back to trunk. (And TCL_ARGV_AUTO_REST macro now works! Found that during testing.)

Download link for version with merge done: https://core.tcl.tk/tcl/zip/Tcl%20Source%20Code-06c0436f0421b3ee.zip?uuid=06c0436f0421b3ee5c7fac0bd919e7dde362d3a6

Downsizing is probably not very important, and should be quick (the memory manager can just note the change in used size in its metadata block; it doesn't need to actually allocate). It does help with subsequent use of valgrind though. :-)

The take-home message: as long as we have some tests, we're good to go.

and a last one: does it make sense to downsize the leftovers / remObjv array?

    leftovers[nrem] = NULL;
    *objcPtr = nrem++;
    *remObjv = (Tcl_Obj **)ckrealloc((char *)leftovers, nrem * sizeof(Tcl_Obj *));

I'd probably pass it back as is:
- ckrealloc might fail and is "relatively" expensive
- memory saving is expected to be relatively low, maximum is : objc * sizeof(Tcl_Obj *), which would typically be some 40 bytes or so.

... but of course this is a serious question of style :)

Thanks for clarification. I have taken dkf's version and ported back to 8.6b1.1, which seems to work as expected. However, I kept the non-memcpy assignment at the end for the sake of readability (instead of performance).

Oops, right. Missed your comment in the flow, sorry.

Not quite. I reviewed the patch and made some small adjustments (hence my comment below about an off-by-one error).

What we need though is a test that detects the problems (possibly only during a memdebug/valgrind run, but even so) so that we can be _sure_ that this is fixed. I've no problem with that requiring more things in tclTest.c if necessary, of course...

To be clear: you can just as well download Donal's commit, which is equivalent AFAICT (right, Donal ?)

Roman, the patch I attached is against 8.6 trunk tip.

It's in fossil on http://core.tcl.tk/ (All our development has been there for about 6 months or so.) Download link...

http://core.tcl.tk/tcl/zip/Tcl%20Source%20Code-8edb5ef770d0de1f.zip?uuid=8edb5ef770d0de1f32ba001970f32a71554f1197

Could you assist? The patch fails for me, and I cannot find the branch in sf cvs:

patch --dry-run --strip 1 < parseargs.patch
patching file tclIndexObj.c
Hunk #1 FAILED at 1111.
Hunk #2 FAILED at 1180.
Hunk #3 FAILED at 1293.

Thanks, R.

Try what I've committed to the bug-3413857 branch; we could do with a test before merging back into trunk. (I think there was an off-by-one bug in the code; for example, if the only thing left was the command name, *objcPtr should only be set to 1, not to 2, as the trailing NULL is not counted.)

Please review attached patch.

File Added - 424492: parseargs.patch

High prio as low-hanging fruit.

BTW, why NULL-terminated the remObjPtr array ? No such contract appears in the manpage, and it is unneeded since the length is transmitted elsewhere.