Tk Source Code

Cascading menus allows the parent menu to be set as a sub-menu. This doesn't make sense and can only be a result of a typo or a bug. To demonstrate this behavior:

  % menu .m
  % . configure -menu .m
  % .m add cascade -menu .m

This hangs wish for a while, seemingly going into an endless recursion, finally resulting in the error message: "No more menus can be allocated."

Expected behavior: give error message instead of going into a loop trying to cascade the menu into itself.

Great, thanks Marc for merging to all branches! Harald

Great ! So, you may merge the revised version. Perhaps also consider the function name change proposed by Christian.

Thanks for all, Harald

After Jan's patch the CI run shows no test failures with just the one
block of code that checks for loops in the menu "tree".

Yes, I will modify the bugfix branch.

The "strange" macOS behavior is that TkMacOSXMenu.c replicates the Tk menu
hierarchy in the native Apple menu hierarchy.  If Tk has a loop in its menu
"tree" then it will be copied to create a loop in Apple's menu "tree".  And
Apple also does not check for these loops.  So the loops cause a crash.  The
difference is that the crash happens first in the Apple code, rather than
in the Tk code.

Christian, thanks!

Task: modify bug branch to only contain the "#ifdef MACOS" loop test and run CI.

Marc: will you modify the bug branch or shall I do it?

THanks for all, Harald

If the CI shows that the single place very early in
ConfigureMenuItem() is sufficient, that's fine for me.
Initially, it started as shot in the dark to fix MacOS
which has a somehow stranger behavior compared to the
other platforms. Maybe some bike shedding is due to
the possibly misleading function name CheckLoop().
Would RejectMenuForRecursiveCascadeUsage() be more
appropriate?

Tests are clean with new test. Only windows fails with a dialog test: https://github.com/tcltk/tk/actions/runs/23182362812/job/67357874157#step:9:255 This is probably unrelated.

Merged test to all branches:

• main: [dc2c9636]
• core-9-0-branch: [690fd9c0]
• core-8-6-branch: [8e2f027d]

Christian: your opinion on Marcs statement, that only the test location of MacOS is required? Activate for all platforms? Remove other tests?

Thanks for all, Harald

My tests indicate that the one guard which is currently compiled only for
macOS is sufficient to prevent loops being created by add cascade.  If that
guard were enabled for all platforms then the other two would be superfluous.

I think this is reasonable.  A loop cannot be created by cloning a menu with
no loops.  The only way we seem to be able to create a loop is by using add
cascade, and that one guard protects add cascade.

Also, Christian, your stance on "formally unnecessary tests" is inconsistent.
If that really were your opinion then you would not have restricted compilation
of the early guard to macOS only.  If you really thought it was better to have
as many guards as possible, even if some of them are redundant, then you would
have enabled the early guard for all platforms.

I vote for removing the "#ifdef MAC_OSX_TK" and its matching "#endif" and
then removing the other two redundant code blocks containing calls to
CheckLoop.

Christian, sorry, I didn't get the usual notification email when you added
your suggested test for the cloning situation, so I didn't see it until now
when I happened to notice action on this ticket.

I will try running that code on macOS and linux shortly.

(I didn't know that clones were *always* created when a menubar is
configured.  I thought you had to tear off a menu, or something like that.)

Harald, my humble opinion is frankly to accept the presence of
a formally unnecessary test (which might be dead code) for the
certainty of having a better (in the less crashing sense) overall
experience. But let the CI decide, literally. In any case did I
prove that a test can be constructed which triggers a crash for
a cloned menu.

Tested the "menu clone" test by Christian manually. It goes in endless loop on 9.0.2. It errors out by the bug branch.

Test is added with commit [dc74bdba] as test menuDraw-16.8

Marc, what I understood is that you say, that only the Mac-only recursive test is required and the other tests may be removed. Any opinions by Christian or Emiliano?

Please use the bug branch for testing by CI.

Thanks for all, Harald

Contribution by Marc on the core list:

My conclusion after looking at the code is that the macOS crash was caused by creating a loop in the macOS native menu hierarchy. That corruption was happening before the Tk recursion test was being done. Christian's fix works by making the Tk recursion test happen earlier, before there has been an opportunity to corrupt the native menu hierarchy by calling TkpConfigureMenuEntry. This is surely just confirming the analysis that led Christian to propose his fix in the first place.

I don't think this indicates any craziness with macOS. But it does suggest some sloppiness on the part of Apple -- they don't bother to check for these sorts of recursions either; they just blindly assume that no one would ever create a loop in their menu hierarchy.

I think that the "early" test for recursion is happening at the correct place. So I would support removing the #ifdef MAC_OSX_TK and do the "early" test on all platforms.

One question that should be asked is: does doing the "early" recursion test mean that it is not necessary to do the later recursion test? (If so, then lines 2202 - 2218 could be removed.) I think the answer is "yes". I tested that on macOS by disabling the code block from line 2022 to 2218 while leaving the early test in place. All menu tests and all menuDraw tests passed. There were no hangs and no crashes.

I think the #ifdef MAC_OSX_TK should be removed, so the early test runs on all platforms, and the later test in 2022 - 2218 should be removed.

CI does not fail any more on Mac-OS since the hotfix by Christian - thanks.

Christian, the proposed test is an additional test to add? I may do that.

Thanks for all, Harald

How about this one?

  menu .mbar -title MENU
  menu .mbar.edit
  .mbar.edit add cascade -label COMMAND -menu .foo
  .mbar add cascade -label CASCADE -menu .mbar.edit
  . configure -menu .mbar
  .#mbar.#mbar#edit entryconfigure last -menu .mbar

I attempted to check whether it is necessary to have all of the current
calls to CheckLoop, or whether having just the one call in ConfigureMenuEntry
would suffice if that were enabled for all platforms.  It seems plausible
to me that preventing loops when configuring a menu would also prevent loops
from appearing when cloning a menu.

On macOS (which is currently the only platform that has the call to
CheckLoops in ConfigureMenuEntry), I removed the other two calls to
CheckLoops (one in ConfigureMenuCloneEntries and one in MenuAddInsert).
The result was that all tests pass.  However, I don't think that any tests
were added for the cloning situation.  And testing with TkChat did not
prove to be a simple task.

Is there any chance that someone could provide a simple example of
how a loop could be created when cloning a menu even when
ConfigureMenuEntry prevents creating a loop?

Nicolas tested the hotfix by Christian manually and it was effective. I am asking myself, if we need the "#ifdef MACOS" or if this code would be ok for all platforms, as it is generic code.

Hotfix by Christian merged to:

• bug branch: [7f575912]
• main: [8551b849]
• core-9-0-branch: [645b5eaf]
• core-8-6-branch: [8551b849]

Lets hope, that this is ok.

Christian proposed to add an "update" before the last command of the test. If the current test hangs, it is a good test, so lets keep it as it is. If the "update" is significant, we could make a 2nd test with the update.

THanks for all, Harald

Seems that MacOS is extra unpleasant. Please try this one:

https://androwish.org/home/info/887cf9013c9d61b1

Sorry for that, it is me again ;-).

Yes, I had seen that. But to my knowledge, MacOS hangs on CI with Tk are the normal behaviour. We already have this with the monotonic clock. I have a talent to break the MAC.

Can anybody with a MAC manually test?

New test is "menuDraw-16.7". I removed test menuDraw-16.8, as it required a manual menu click on windows.

Thanks, Harald

I'm sorry, but the MacOS builds for all versions (8.6, 9.0 and trunk) are broken now because of this changed :-(

menuDraw.test is hanging:

....
main.test
menu.test
menuDraw.test
Error: The action 'Run Tests' has timed out after 30 minutes.

CI clean! Great!

Committed to branches:

• main: [b661380c]
• core-9-0-branch: [d43efad5]
• core-8-6-branch: [f620dfde]

The announced man page change is not performed, due to missing input.

Thanks to Christian and Emiliano for this great work.

Take care, Harald

Merge to main procedure starts:

• Close last checkin [8ba382be] of branch [7f67bb4054d6d7d9-menu-recursive]
• Merge main to branch to [7f67bb4054d6d7d9-menu-recursive-chw] by checkin [6d8ff452].
• Cherrypick the tests by Emiliano by checkin [cdc2f60d]
• Correct tests [dd480a4d]: menuDraw-16.7: changed error message, menuDraw-16.8: removed, as it requires a manual click on the menu
• Run CI, result tomorrow...

Thanks for any comments, Harald

Thanks, Emiliano!

   *   Can we adjust the docs to the implementation?
   *   Can we merge the test from the other bug branch?

Thanks,
Harald

Agree with Christian here. Things are better now, and while I'm mildly inclined to force the parent/children relationship as described in the docs, this would cause scripts breakage. And I do value backwards compatibility more than correctness.

Harald,

from my point of view you can merge the loop check fix, its benefits are:

* no need to enforce parent/child relationships amongst menu cascades
* thus, no need to change current test scripts
* no more loops possible which lead to known problems
* no more loops possible which could expose further still unknown problems

Emiliano, Christian, others, do we have an opinion, if this could be merged to main?

Thanks for all, Harald

Thanks, Emiliano, for the proposal. Thanks, Christian, for the patch:

• [b153c338] imports https://androwish.org/home/info/8a4ac34a18e35225

Take care, Harald

Emiliano,

Your first point is IMHO a problem, since it allows to build the loop
in the internal tkMenu.c data structures and delays its detection 
until something has to be drawn on screen. Can we be sure, that the
presence of the loop does not trigger other disastrous effects under
the hood? Or isn't it better to disallow the loop beforehand?

Your second point is addressed in this check-in:

  https://androwish.org/home/info/8a4ac34a18e35225

Yes, pretti obvious, empty if body.

Commited:

• [ad020601] imported from https://androwish.org/home/info/762d0543978de877

Thanks, Harald

Sorry, Harald, I've messed up an if-block, see

  https://androwish.org/home/info/762d0543978de877

and mind the masterMenuPtr vs. mainMenuPtr.

Ok, name changed. Now it compiles and all tests pass on Windows.

THanks for all, Harald

Harald,

so my diff is a victim of one of our brilliant renaming orgies.
The offending structure field once upon a time was masterMenuPtr,
but this wasn't DEI enough and required a rename to mainMenuPtr.

Bummer!

New branch "7f67bb4054d6d7d9-menu-recursive-chw" contains the two AndroWish commits:

• [5203e5c7] Imports https://androwish.org/home/info/d68d0565987487b6: needed to add the interp pointer to function ConfigureMenuCloneEntries to compile
• [16410993] Imports https://androwish.org/home/info/69ad1d084dd49905: this does not compile. The error is as follows:

generic\tkMenu.c(2039): error C2039: "masterMenuPtr" is not a member of "TkMenu".

Perhaps the mentioned thierd patch is missing or I did something wrong...

No, Emiliano's fix is not included. Since nothing is needed
to enforce parent/child relationship between the involved
cascade menus. My goal is as explained to definitely not
enforce is, since otherwise as you saw the test code needs
to be modified to account for that fact.

Thanks, great.
The 3 diff snippets include the fix by Emiliano?

Thanks,
Harald

@emiliano, indeedly, the clonery suffers from the same loop blues.

Therefore, my loop check now goes after clones, too, see

  https://androwish.org/home/info/a9d17fe0bddbdf6e

@oehhar, please take the three diff snippets over in your bug fix branch.

My overall feeling now is, that we could leave the menu module
without enforcing relationship, when we have the proper loop check
in place. The bonuses are, that no existing tests need be modified,
and we have eliminated one more option to FUBAR a Tk app or even
an entire system.

Hi Emiliano,

if it helps, I can puit https://androwish.org/home/info/d68d0565987487b6 in a branch based on Tk main branch.

I usually take the Androwish diff, then search the relevant place and copy it over line by line. There are also often other transformations pending like different variable names and types.

THanks for all, Harald

% .mbar.edit entrycget last -menu
.mbar.edit.aa
# now the clone, created as part of [. configure -menu .mbar]
% .#mbar.#mbar#edit entrycget last -menu
.#mbar.#mbar#edit.#mbar#edit#aa
# now, break things up
% .#mbar.#mbar#edit entryconfigure last -menu .mbar

And that quote makes me suspicious:

  The menu docs clearly states that "The associated menu must be a child of the menu containing the cascade entry"

Why is this, when the code does not enforce it?

Maybe because it was not addressed in it consequences ever.

Bummer!

Hello friends,

as usual I'd like to disagree with due respect.

The problem is two fold:

* Tk in its current state does not enforce relationship among menus
* ditto does not look out for recursion and loops

Thus it is possible to write

  menu .m
  menu .m2
  menu .m3
  .m add cascade -menu .m2
  .m2 add cascade -menu .m3
  .m3 add cascade -menu .m
  .config -menu .m

and end up in whatever disaster the system decides to crash.

The real blues is the loop. So this needs to be addressed.
My humble proof-of-concept in this regard can be seen in

  https://androwish.org/home/info/d68d0565987487b6

which might (testing and approval needed) prevent from doing
nasty loopery on

  "<menu> add cascade -menu <deadly-loop>"

or

  "<menu> entryconfigure ... -menu <deadly-loop>".

These seem to be the only two entries into creating deadly loops.

Added tests that exercise the two checks

Great! Test on Windows ok too. Thanks, Harald

Already commited to the bug branch. All menu tests pass on X11.

Thanks, Emiliano, I appreciate.
I am only the moderator, saving code snippets, removing manually the "+" from the diff, copying code snippets from undrowish, testing with ms-vc on windows etc. I have no opinion.
Any action is great.

Thanks for all,
Harald

I get the following test failures with the patch applied:

==== menu-3.54 MenuWidgetCmd procedure, "postcascade" option FAILED
==== Contents of test case:

    menu .m1
    menu .m2
    .m1 add cascade -menu .m2 -label "menu-3.57 - hit Escape"
    .m1 postcascade 1
    .m1 postcascade {}

---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: cascade menu ".m2" is not a child of ".m1"
    while executing
".m1 postcascade 1"
    ("uplevel" body line 5)
    invoked from within
"uplevel 1 $script"
---- errorCode: TK MENU CASCADE NOCHILD .m1
==== menu-3.54 FAILED

menuDraw.test


==== menuDraw-16.3 TkPostSubMenu FAILED
==== Contents of test case:

    menu .m1
    .m1 add cascade -label test -menu .m2
    .m1 postcascade 1

---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: unknown cascade menu ".m2"
    while executing
".m1 postcascade 1"
    ("uplevel" body line 4)
    invoked from within
"uplevel 1 $script"
---- errorCode: TK MENU CASCADE UNKNOWN .m1
==== menuDraw-16.3 FAILED

Thank you, Harald

Thanks. Patch now in [951260f4] in branch [7f67bb4054d6d7d9-menu-recursive]

Harald

The menu docs clearly states that "The associated menu must be a child of the menu containing the cascade entry"

However, the submenu posting code never checks this requirement.

The attached patch checks that, when calling "$menu postcascade $index", the associated submenu exists and is a child of the menu containing the cascade entry, raising errors if these requirements are not met.

On windowswith 9.0.3, the process is terminated with a syslog APPCRASH message. Clearly a bug. Thanks, Harald