Tcl Source Code

gdb backtrace (linux x86_64)

Program received signal SIGILL, Illegal instruction.
Tcl_Panic (format=0x7ffff7e35c68 "alloc: invalid block: %p: %x %x %x")
    at /home/emiliano/src/tcl/generic/tclPanic.c:108
108	    __builtin_trap();
(gdb) bt
#0  Tcl_Panic (format=0x7ffff7e35c68 "alloc: invalid block: %p: %x %x %x")
    at /home/emiliano/src/tcl/generic/tclPanic.c:108
#1  0x00007ffff7da4026 in Ptr2Block (ptr=0x5555556540f0)
    at /home/emiliano/src/tcl/generic/tclThreadAlloc.c:811
#2  0x00007ffff7da33e7 in TclpFree (ptr=0x5555556540f0)
    at /home/emiliano/src/tcl/generic/tclThreadAlloc.c:385
#3  0x00007ffff7c69c5b in Tcl_Free (ptr=0x5555556540f0)
    at /home/emiliano/src/tcl/generic/tclCkalloc.c:1178
#4  0x00007ffff7e0ec59 in TclBN_mp_clear (a=0x7fffffffdac0)
    at /home/emiliano/src/tcl/libtommath/bn_mp_clear.c:12
#5  0x00007ffff7d136a6 in TclCompareTwoNumbers (valuePtr=0x5555556012d0, 
    value2Ptr=0x5555556012d0) at /home/emiliano/src/tcl/generic/tclExecute.c:9265
#6  0x00007ffff7c5e9b9 in ExprMaxMinFunc (dummy7587=0x0, 
    interp=0x5555555a96e0, objc=2, objv=0x5555555ad790, op=1)
    at /home/emiliano/src/tcl/generic/tclBasic.c:7617
#7  0x00007ffff7c5ea50 in ExprMaxFunc (dummy7628=0x0, interp=0x5555555a96e0, 
    objc=2, objv=0x5555555ad790) at /home/emiliano/src/tcl/generic/tclBasic.c:7634
#8  0x00007ffff7c58d43 in Dispatch (data=0x555555601368, 
    interp=0x5555555a96e0, dummy4602=0)
    at /home/emiliano/src/tcl/generic/tclBasic.c:4641
#9  0x00007ffff7c58dae in TclNRRunCallbacks (interp=0x5555555a96e0, result=0, 
    rootPtr=0x0) at /home/emiliano/src/tcl/generic/tclBasic.c:4657
#10 0x00007ffff7c5bebc in TclEvalObjEx (interp=0x5555555a96e0, 
    objPtr=0x55555556ab10, flags=131072, invoker=0x0, word=0) at /home/emiliano/src/tcl/generic/tclBasic.c:6116
#11 0x00007ffff7c5be4c in Tcl_EvalObjEx (interp=0x5555555a96e0, objPtr=0x55555556ab10, flags=131072) at /home/emiliano/src/tcl/generic/tclBasic.c:6097
#12 0x00007ffff7d1f7b7 in Tcl_RecordAndEvalObj (interp=0x5555555a96e0, cmdPtr=0x55555556ab10, flags=131072) at /home/emiliano/src/tcl/generic/tclHistory.c:182
#13 0x00007ffff7d636b7 in Tcl_MainEx (argc=-1, argv=0x7fffffffdfc8, appInitProc=0x55555555523c <Tcl_AppInit>, interp=0x5555555a96e0)
    at /home/emiliano/src/tcl/generic/tclMain.c:523
#14 0x000055555555523c in main (argc=1, argv=0x7fffffffdfc8) at /home/emiliano/src/tcl/unix/tclAppInit.c:98

Proposed fix in branch bug-8dd28070

I think the solution is more complicated than that.

Tcl_GetNumberFromObj() documentation:

 * Side effects:
 *      Can allocate thread-specific data for handling the copy-out space for
 *      bignums; this space is shared within a thread.

So you cannot call Tcl_GetNumberFromObj() twice in a row without copying the result in between ..... That looks to be the problem here.

> it looks more complicated to me.

Sergey, did you mean my latest commit or the original proposed fix?

Sergey, as to why your example does not go boom, I would guess it is because the additional reference count from the variable reference x means Tcl_TakeBignumFromObj does not change the bignum internal rep because it is now shared.

I meant it generally. I don't understand why a comparison of bignum (compared with itself) handles different... not to mention why `max` (with single argument) is compiled in this way.

> I think the solution is more complicated than that

Some confusion here. I didn't say that Ashok's solution was too complicated. I intended to say that the correct solution should be more complicated that a simple if-statement. :-)

About Sergey's example not going kaboom "...because the additional reference count...", note this also crashes

$ tclsh9.1 
% set a -668336881543038127783364011867
-668336881543038127783364011867
% expr {max(abs($a))}
alloc: invalid block: 0x5b63c9713780: 0 0

I'm confused by what commit the comments here are referring to.

Does anyone see problems with the tip of the [bug-8dd28070] branch? It passes both the original report as well @emiliano's example.

Regarding Sergey's and Emiliano's query - in Sergey's case, the passed in valuePtr (== value2Ptr) have reference counts of 3. Does the Tcl_Obj is not emptied by Tcl_TakeBignumFromObj. In the original report and Emiliano's expr {max(abs($a))} example, the reference count is 1, so the object gets emptied on value1 resulting in a crash on value2 manipulation.

> Does anyone see problems with the tip of the [bug-8dd28070] branch?

My comment was directed at your first commit, adding the if() statement only. I didn't have a full look at your later commits, but it matches the direction I would it expect it to go. So, just go ahead!

I decided to go with the fix in [bug-8dd28070-alt] which simply replaces the Tcl_Take* calls with Tcl_Get* calls. It's less code changes and also more accurate a fix for the actual problem. Will merge once GH CI passes.

Well, it is basically a nonsense that there is a public TAKE function which modify the object in this way, and still worse all that depending on objects refCount (only). I'm not against the release of the internal representation of object - normally a change of internal representation may indeed happen (even for shared objects, for instance if object shimmers), but then the string representation still contain original value, so if internal representation gets needed again, it'd be reconstructed again. Again, freeing of internal representation on some object without string representation is bad idea at all (regardless it is shared or not) and shall be always avoided. The emphasis is on always. Just because it modifies the object drastically and because it violates EIAS concept.

Therefore this issue is just an aftereffect and may happen in other cases by usage of Tcl_TakeBignumFromObj() function (also on some extensions).

I have an alternative solution for this:

• 8.6 related fix for Tcl_TakeBignumFromObj() - [57ed005b52d3d2c9]
• 9.0 related fix for ExprMaxMinFunc() (and Tcl_TakeBignumFromObj()) - bug-8dd2807066d7-9.0-minmax

@sebres, I agree with your analysis. I think it's possible to construct a testcase demonstrating this cornercase, but even without such testcase I agree.

What a teamwork! :-)

I also agree with Ashok's proposal. The reason two GetNumberFromObj() calls after each-other do no harm in this case is because the returned value is not used any more. The actual value is later obtained by another Tcl_GetBignumFromObj() calls.

So, +1 on all suggestions.

Hm .... wait a bit.

Thinking more about it, Tcl_TakeBignumFromObj() should only be used when the Tcl_Obj is Tcl_DecrRefCount'ed immediately. In that case, the current implementation makes sense.

So, if - ever - the string-representation is lost, it means Tcl_GetBignumFromObj() should have been used in stead. The bug in this ticket is an example of the wrong Tcl_TakeBignumFromObj(). The more I think about it, the more it makes sense.

Fixed [ce87baf3c7a1ecf1|here]

Let's discuss the changes in Tcl_TakeBignumFromObj() further.

> That is not an appropriate fix

Well, it fixes the reported problem. But I don't intend to stop here, don't worry.

Just give me some time .....

In Tcl 8.6, it's intentional that TclCompareTwoNumbers is destructive: It is only used in the bytecode engine, when two numbers are popped of the stack, and the compare result is pushed back on the stack. As long as that's the only place it's used, that should be OK.

On 9.0, the function is re-used for the max/min functions which were re-implemented in C. That's where the problem started.

> Let's discuss the changes in Tcl_TakeBignumFromObj() further.

Agree.

> I still prefer to simply replace Take with Get instead and not require any changes at the call site in `ExprMaxMinFunc`.

It'd be unneeded if Jan had merged the whole fix of me, but...

> A new bignum has to be created in either case.

Well, I think this is exactly that, what Jan trying to achieve or rather to assume with "the current implementation makes sense" - the hope is that bignum can be moved from object to bignumValue pointer if it is unshared (so it'd be not created)... But, IMHO it is not correct - even with refCounts 1 (or 0), one can't guarantee that the object will be unused hereafter (or as Jan wrote "Tcl_TakeBignumFromObj() should only be used when the Tcl_Obj is Tcl_DecrRefCount'ed immediately"). So, IMHO, it is not correct to rely on refCounts in public function (however I'm OK if it'd happen always and panic if shared).

Just it is often the case that the objects supplied to tcl::mathfunc::* are shared (via arguments etc), so would be copied then.

In any case, I don't understand why we can not use the value of objPtr->internalRep.wideValue directly for the comparison - shimmer is unexpected there, and the copy on read access is a bit unexpected to me therefore.

> But, IMHO it is not correct - even with refCounts 1 (or 0), one can't guarantee that the object will be unused hereafter

I'm trying to create a test-case, showing the remaining problem (if there is one), but I'm not getting very far. Something like:

$ tclsh9.0
% tcl::unsupported::representation [expr {max(9999999999999999999999999999+1,0)}]
value is a bignum with a refcount of 2, object pointer at 0xa0009c0c0, internal representation 0xa000dfc30:0x100002, no string representation
% puts [expr {max(9999999999999999999999999999+1,0)}]
10000000000000000000000000000

This works as expected. Any idea?

$ tclsh9.0
% tcl::unspupported::representation [expr {max(abs(-668336881543038127783364011867),0)}]
invalid command name "tcl::unspupported::representation"
% tcl::unsupported::representation [expr {max(abs(-668336881543038127783364011867),0)}]
value is a pure string with a refcount of 1, object pointer at 0xa000a02e0, string representation ""

Got it! Thanks!

[780d2f3375e35f2c] illustrates what I meant with unneeded CoR (copy on read)... This improves TclCompareTwoNumbers by usage of internal representation of bignum representations of objects during copmpare process without to copy/free them.

Additionally it is also 4x faster than with a copy:

  % apply {{} { set x [expr 2**64]; set y [expr 2**65]; timerate { expr {$x < $y} } }}
- 0.235482 µs/# 3729340 # 4246611 #/sec 878.192 net-ms
+ 0.061164 µs/# 10849804 # 16349472 #/sec 663.618 net-ms
  % apply {{} { set x [expr 2**64]; set y [expr 2**65]; timerate { expr {max($x,$y,$x,$y)} } }}
- 0.970795 µs/# 996554 # 1030083 #/sec 967.450 net-ms
+ 0.216824 µs/# 4035058 # 4612032 #/sec 874.898 net-ms

I'd like to merge it later if no objections follow.

> I can imagine a possibility that if an existing allocation from a previous call is too big for a smaller bignum, it is reallocated

Yes, but it'd be the pointer stored in mp_int::dp, so quasi pointer in pointer (of ptr1/ptr2), whereas ptr1/ptr2 themselves always point to the same address in TSD (per thread).

> In which case ptr1 may be != ptr2 but no longer even points to valid storage.

No, it cannot. TSD is allocated once per thread and remains unchanged for every static anchor. If GetNumberFromObj returned different pointers one of them is not a bignum, if they are equal - both are bignum and the last win.

This variant (well, a bit more complex) works on my side already dozen years without any issue. However I think it'd be better to backport more from there, e. g. yet another GetNumberFromObj to avoid getting bignum in TSD (just a performance thing) and then one could always use TclUnpackBignum for local variable for any constant accesses to bignum type. This way one could eliminate almost every Tcl_GetBignumFromObj/Tcl_TakeBignumFromObj invocation and improve the performance drastically.

I don't have a basic objection against @sebres' suggestion. A factor 4x seems worth it. Maybe an interal TclGetNumberFromObjEx() with points to a user-defined space (a union of int, double and mp_int) in stead of a Tcl-provided space would be a minor improvement.

But that can be done later. It would be nice to have this improvement in Tcl 9.1a1, so +1 from me.