Tcl Source Code

apnadkarni added on 2026-03-06 11:22:03:

Also present in core-9-0-branch since [24d8933cbd]. Proposed fix at [edef37bbf6].

The change of the ScriptLimitCallbackKey.type field from long to int introduced trailing pad bytes in a structure used as a key in hash tables. Uninitialized pad bytes are a no-no for structs used as hash keys.

Now changed to be intptr_t which should not result in pad bytes as it will always be same size as a pointer on all known platforms.

apnadkarni added on 2026-03-08 04:07:15:

Fixed in [58acf5656f].

jan.nijtmans added on 2026-03-09 07:11:51:

Good catch! Solution works.

However, if I may .....(Sorry!). Two remarks:

1) In Tcl 8.6, the "type" field still is a long, which is 32-bits on Windows 64. So, the same problem is there too.

2) I don't think the presence of the pad bytes is the problem. The hashtable initialization is! If you calculate the size as "sizeof(ScriptLimitCallbackKey)/sizeof(int))", the pad-bytes are counted as well, that's the real problem.

Fixed that [1b6b19bd5561efc0|here], for 8.6 too. Are you OK with forwarding this solution to 9.0 and 9.1 too?

apnadkarni added on 2026-03-10 17:41:04:

jan.nijtmans added on 2026-03-10 18:35:10:

> I don't see any particular advantage of your solution

The advantage of my solution is that the type field can be accessed with a 32-bit access, not with a 64-bit access, and all hash-key compares only have to compare 3 integers not 4! That's surely better. Hoping you see it now.

> First, the C standard does not guarantee the 4 pad bytes appear as trailing bytes.
Hmmm. You are wrong here. Padding always concerns trailing bytes!

> I really do not like this kind of violation of compiler contracts no matter whether there is any practical effect.
Sorry, wrong again. On a 64-bit platform, which requires 64-bit values to be properly aligned, all mallocs are aligned on 8-byte boundaries.

So, please don't invent problems which don't exist.

apnadkarni added on 2026-03-11 02:53:59:

jan.nijtmans added on 2026-03-11 10:45:12:

Noted! Thanks!

sebres added on 2026-03-11 15:40:45:

> It has nothing to do with allocation strategy.

I'm agree. However it is indirectly about allocation, see below...

> It's a compiler expectation that (ScriptLimitCallbackKey *) will point to a location with sizeof(ScriptLimitCallbackKey) valid bytes

Nope. The compiler expectation here is to allocate, compare, hash, whatever action with an array of int's, and although it allows so to handle structures in a hash-table, it doesn't do that using structures internally, because the hash-table API considers blocks of int's, nothing else.

So if you want to estimate the impact of the change of Jan, you shall rather consider it as array of ints everywhere. But!..

The fix of Jan may indeed introduce a problem indirectly, since AllocArrayEntry could allocate a block for hash-entry + 3 integers (12 bytes), it can indeed cause many issues at corner case (e. g. ckalloc returns last block matching the range of buffer). And in best case an UB, since casting a struct that requires 16 bytes (due to 4 bytes of trailing padding) to a memory block of only 12 bytes may lead to UB by an attempt to access the struct as a whole or to use it in certain operations.
Of course it depends, but in worse case it can be even out-of-bounds memory access (with SF or data corruption), BO on write (in assumed as padding memory), broken array pointer arithmetic etc.

So I am with Ashok here and it shall be rewritten (at least for 64-bit target).

jan.nijtmans added on 2026-03-11 15:58:45:

Wasting too much time in this kind of discussion, but ....

> ... may lead to UB by an attempt to access the struct as a whole or to use it in certain operations

The struct is _never_ accessed as a whole. Inside the Tcl_HashTable it is always accessed as 3 integers. Outside the Tcl_HashTables, there is no way to access the padding storage in the struct.

If there's something wrong, valgrind should be able to show that.

Sorry!

sebres added on 2026-03-11 16:33:13:

> Outside the Tcl_HashTables, there is no way to access the padding storage in the struct.

But there is it, right now, see for instance TclRemoveScriptLimitCallbacks.

Don't forget that optimization is compiler related thing and... Just imagine it'd write a byte code using 64-bit instruction to load keyPtr->type to 64-bit register (in order to compare its lower 32-bits later)... What would happen if this would be on range of allocated memory?

Also it may do that later by some changes if we'd get more of such casts. Much worse if it'd be the write case.

I know that it is broken by design, but lets not make it more faulty as it is.

jan.nijtmans added on 2026-03-11 16:37:29:

> But there is it, right now, see for instance TclRemoveScriptLimitCallbacks.

I see keyPtr->interp and keyPtr->type being accessed, that's not the structure 'as a whole'.

sebres added on 2026-03-11 20:26:26:

jan.nijtmans added on 2026-03-12 07:57:42:

Well, just for the fun, I checked the assembler generated (both with gcc and clang on ARM64, maximum optimization -O3). It looked like:

        mov     x1, x0
        ldr     x0, [x0]
        ldr     w1, [x1, 8]
        b       Tcl_LimitRemoveHandler(void*, int)

Conclusion: the optimization you suggest, which I don't think is valid, isn't done by either gcc or clang. If - for the fun - the type of "type" is changed to ptrdiff, I get:

        ldp     x0, x1, [x0]
        b       Tcl_LimitRemoveHandler(void*, long)

so the compiler knows the ldp instruction.

I did another experiment, forcing 64-bit access in this exact place. The result was a crash in testcase interp-34.7. So whenever a compiler does this (in my view) invalid optimization, we have a testcase for it, which triggers consistently. I'm sure that, whenever a compiler introduces this optimization, Tcl will not be the only project broken. (It would not be the first time this happens)

sebres added on 2026-03-12 10:28:26:

OK, now I'm agree with Ashok - you don't understand what the issue is. If today the compiler doesn't optimize it using LDP instruction yet, it can do that later, because this will be not "broken" as you said, but fully legitimate thing - the compiler knows that the structure has a padding (basically it is 16 bytes large), so LDP instruction would be fully correct here. It is strange to push this on the fact that it is not currently in use. Maybe simply not yet? Or not in this optimization level, but within -O3 etc. Or simply by certain circumstances...

But what it surely unable to know, is that we had casted it to a 12-byte block of memory (since this allocation happens in another files). This is definitively an UB.

Even a known and still working UB remains UB, and it is weird to listen you decided to retain it... Not to mention you're trying to rely to some test coverage. Test coverage of the UB?! Really?

This is calling bomb-planting, Jan... And saying that I get dejavu feeling.

jan.nijtmans added on 2026-03-12 10:42:28:

So, let's agree we disagree then. You think this would be a valid (future) optimization. I think it won't be a valid optimization at all, since it would do read access of uninitialized padding bytes.

I wasted enough time on this discussion already.

sebres added on 2026-03-12 13:13:23:

apnadkarni added on 2026-03-13 03:17:42:

jan.nijtmans added on 2026-03-13 08:11:02:

apologies accepted :-)

The rule "better safe than sorry" applies here too. The performance difference between the two solutions is negligible anyway, it's not worth further discussion. I learned a lot (I never heard of the the ARM ldp instruction ...)