Tcl Source Code

LLVM Clang 17 makes -fsanitize=function available for C (previously it was just for C++). It is implied by -fsanitize=undefined. It complains about calls to freeProc(clientData) from Tcl_EventuallyFree() and Tcl_Release(). Unlike warnings from e.g. -Wincompatible-function-pointer-types, this UBSan error is not suppressed by casts to (Tcl_FreeProc *). Example when exiting tclsh:

tcl/generic/tclPreserve.c:229:3: runtime error: call to function DeleteInterpProc through pointer to incorrect function type 'void (*)(char *)'
tclBasic.c:1422: note: DeleteInterpProc defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior tcl/generic/tclPreserve.c:229:3 in 

> ... LTO which I am not aware how well Tcl supports

Tcl and Tk support LTO, except for the stub libraries (for which LTO actually doesn't make much sense).

I have since learned that eliminating -fsanitize=function complaints is also useful for security hardening, because some control flow integrity schemes require calling functions using the correct pointer type (although some schemes also rely on LTO which I am not aware how well Tcl supports). See https://maskray.me/blog/2022-12-18-control-flow-integrity#fsanitizefunction

There's a lot of code out there (e.g. Tk, Itcl) which uses this feature, UBSan just reports false-positives for them.

One can use -fsanitize=undefined -fno-sanitize=function, but I doubt UBSan’s reports of undefined behavior are false positives*, despite things behaving as expected under typical calling conventions. Or have I misunderstood what you meant by “false positives”?

*(The only potential false positives I am aware of are those due to this feature’s reliance on name mangling, which is more strict than standard C type compatibility; see https://reviews.llvm.org/D148827.)

That being said, I do not know how immediate any danger posed by optimizing compilers is for exploiting this type of undefined behavior. Instances within a single translation unit, or with LTO enabled, speculatively seem more concerning.

> …Currently, I believe it would be better to not suppress those compile-time complaints, particular

Would it be possible to disable those when doing your UBSan tests? There's a lot of code out there (e.g. Tk, Itcl) which uses this feature, UBSan just reports false-positives for them. As it is now, they can use whatever pointer-type without warning. In Tcl 9.0, the pointer type changes to "void *", there we want the full UBSan warnings, but for 8.x it really doesn't matter much to chase this.

Thanks for all your efforts! I see the point!

-Wcast-function-type also works.

The casts in tclDecls.h cause Tcl_EventuallyFree() and Tcl_SetResult() to be called from function pointers of incorrect type, and so now UBSan complains even more. A couple of examples:

tk/generic/tkMenuDraw.c:796:2: runtime error: call to function Tcl_EventuallyFree through pointer to incorrect function type 'void (*)(void *, void *)'
tclPreserve.c:265: note: Tcl_EventuallyFree defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior tk/generic/tkMenuDraw.c:796:2 in 

tcl/generic/tclTest.c:1117:2: runtime error: call to function Tcl_SetResult through pointer to incorrect function type 'void (*)(struct Tcl_Interp *, char *, void *)'
tclResult.c:419: note: Tcl_SetResult defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior tcl/generic/tclTest.c:1117:2 in 

…Currently, I believe it would be better to not suppress those compile-time complaints, particularly since they help identify and fix would-be -fsanitize=function errors…

-Wcast-function-type also works.

How are the changes to tclDecls.h beneficial?

How are the changes to tclDecls.h beneficial? They ensure the suppression of compiler warnings (or errors as of LLVM Clang 16, by default) regarding passing of incompatible function pointers. Currently, I believe it would be better to not suppress those compile-time complaints, particularly since they help identify and fix would-be -fsanitize=function errors, at least for the iterative workflow I used (remove any existing casts to (Tcl_FreeProc *) by Tcl_EventuallyFree() users, then fix declarations in response to compiler complaints of passing incompatible function pointers, then fix definitions in response to compiler complaints of declaration/definition mismatch).

Thanks for your feedback!

I was aware that the extra (void *) cast would not foll ubsan, but I thought it was worth to give it a try. I'll fix it as you suggest

Ticket title revised.

Jan, I am sorry if the title I gave this ticket was misleading, but unfortunately the work so far on the bug-8ab8a138c9 branch ultimately does not solve the problem.

What matters is the type of actual function declarations/definitions, and not the type of function pointers passed to Tcl_EventuallyFree().

At runtime, when freeProc() is called from Tcl_EventuallyFree() or Tcl_Release(), UBSan -fsanitize=function checks whether the function pointed to by freeProc was actually declared/defined with a signature matching the type of freeProc, which is Tcl_FreeProc. It does not matter to this UBSan check whether the function pointer in freeProc was originally passed to Tcl_EventuallyFree() through any casts, so I do not see what is accomplished by additional intermediate casting through void *.

I still believe the correct fix for this problem is to declare/define functions correctly, which is what I meant in the title: only pointers to functions whose actual declarations/definitions match Tcl_FreeProc should be passed to Tcl_EventuallyFree() (which can be done without casts), so that UBSan does not complain when the function is later called from Tcl_EventuallyFree() or Tcl_Release().

Proposed patch for Tk 8.6: https://core.tcl-lang.org/tk/info/d96974d99d72

I now notice that in Tcl 9, Tcl_FreeProc expects a void *. If reverting to char * is undesirable, then should a macro or typedef be introduced for use by Tk 8.7 and anything else intending to build against both Tcl 8 and 9?