Tk Source Code

View Ticket
Login
Ticket UUID: be8f5b9fc2f6ef25aa98c58a645fa5278029ffe
Title: Tk Menu empty string for `-type` segfaults
Type: Bug Version: 8.6.12
Submitter: E-Paine Created on: 2021-12-06 16:10:10
Subsystem: 10. Generic Menus Assigned To: fvogel
Priority: 5 Medium Severity: Important
Status: Closed Last Modified: 2021-12-11 16:44:28
Resolution: Fixed Closed By: fvogel
    Closed on: 2021-12-11 16:44:28
Description:
In Tk 8.6.12, setting the `-type` option of a Tk menu, causes it to segfault:
menu .m -type {}

Using `configure` does not cause it to segfault:
menu .m
.m configure -type {}

On Tk 8.6.11, it appeared to be silently ignored (was this correct behaviour?).
User Comments: fvogel added on 2021-12-11 16:44:28:
Non-regression test menu-39.1 now merged into core-8-6-branch and trunk.

fvogel added on 2021-12-11 08:00:27:

For the record, the discussion in this ticket prompted the idea exposed in TIP #613.


fvogel added on 2021-12-08 23:06:03:
Thank you for this information regarding the context in which the issue was found, this is always interesting.

I have now committed a non-regression test (menu-39.1).

Besides, I have looked at all (remaining) occurrences of parameters of type TK_OPTION_STRING_TABLE having the TK_OPTION_NULL_OK flag set in Tk. There are exactly four such cases: the -tabstyle and -wrap options of the text widget, the -compound option of the ttk::button, and the -compound option of tabs of the ttk::notebook widget. I have tested each of them and have found that the {} case passed to the option is already correctly handled for each of them so there is nothing more to fix I think.

E-Paine added on 2021-12-08 17:03:28:
Similar to https://core.tcl-lang.org/tk/tktview/46c2f088, this came to light
because of https://bugs.python.org/issue45436. The CPython tkinter test suite has
not raised any other issues with empty strings being passed as an option value,
but I don't know how comprehensive the suite is.

fvogel added on 2021-12-07 14:14:29:
OK, that's fine.

Before closing this ticket I believe we should check whether the same issue could hit from elsewhere in the code. This boils down to looking at where TK_OPTION_STRING_TABLE is used in the code and for which parameter, and check for presence of the TK_OPTION_NULL_OK flag in these parameters declarations.

Also a non-regression check test for the present ticket would not hurt.

I'll deal with the two points to above.

jan.nijtmans added on 2021-12-07 10:38:32:

Thinking about it more, I like your (@fvogel) fix more (at least for 8.6). So, merged now. Thanks!


jan.nijtmans added on 2021-12-06 22:08:19:

Alternative fix [d0b89a43de0b3da|here]. Since this fix changes behavior (although all test-cases pass...) I prefer your fix for 8.6. For 8.7 we could consider making it more consistent.


jan.nijtmans added on 2021-12-06 21:42:04:

My guess is that with this commit it started failing: [bc258ad6eae49a08]

Explanation: The TK_OPTION_STRING_TABLE option never supported TK_OPTION_NULL_OK, so it broke when TK_OPTION_NULL_OK was implemented for TK_OPTION_STRING_TABLE.

Another possible fix would be to accept NULL in the code, it would mean that "" is no longer ambiguous but synonym with "normal". That would be a change in behaviour, but it would be consistent with ".m configure -type" accepting the empty string as being equivalent with "normal" as well.


fvogel added on 2021-12-06 21:16:22:

Thank you for your report!

Commit [94687a026d] fixes this, but before merging it I would like to understand how come it worked in previous versions of Tk, whic I have not yet figured out yet.

I have checked that 8.6.9 for instance returns the correct answer:

ambiguous type "": must be normal, tearoff, or menubar