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: (text/x-fossil-wiki) For the record, the discussion in this ticket prompted the idea exposed in [https://core.tcl-lang.org/tips/doc/trunk/tip/613.md|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: (text/x-fossil-wiki) 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: (text/x-fossil-wiki) 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: (text/x-fossil-wiki) 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: (text/x-fossil-wiki) 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: <code>ambiguous type "": must be normal, tearoff, or menubar</code> |