tux4kids / t4kcommon

GNU General Public License v3.0
4 stars 11 forks source link

Fix seg fault #19

Closed matlacki closed 10 months ago

matlacki commented 10 months ago

When clicking on penguin's face in tuxtype, a seg_fault is triggered, below gdb's output:

Thread 1 "tuxtype" received signal SIGSEGV, Segmentation fault. 0x00007ffff6ea05a2 in T4K_RunMenu (index=0, return_choice=false, draw_background=0x5555555828a8 , handle_event=0x555555582e0f , handle_animations=0x555555583064 , handle_activity=0x555555585427 ) at ../../t4kcommon/src/t4k_menu.c:1058 1058 if (menu->submenu[loc + menu->first_entry]->desc == NULL) (gdb) by Undefined command: "by". Try "help". (gdb) bt

0 0x00007ffff6ea05a2 in T4K_RunMenu

(index=0, return_choice=false, draw_background=0x5555555828a8 <DrawTitleScreen>, handle_event=0x555555582e0f <HandleTitleScreenEvents>, handle_animations=0x555555583064 <HandleTitleScreenAnimations>, handle_activity=0x555555585427 <handle_activity>) at ../../t4kcommon/src/t4k_menu.c:1058

1 0x0000555555585425 in run_menu (which=MENU_MAIN, return_choice=false) at ../../tuxtype_lacki/tuxtype/src/menu.c:82

2 0x0000555555585c9e in RunMainMenu () at ../../tuxtype_lacki/tuxtype/src/menu.c:257

3 0x00005555555827fd in TitleScreen () at ../../tuxtype_lacki/tuxtype/src/titlescreen.c:316

4 0x000055555556abe6 in main (argc=1, argv=0x7fffffffd988) at ../../tuxtype_lacki/tuxtype/src/main.c:219

(gdb) f 4

4 0x000055555556abe6 in main (argc=1, argv=0x7fffffffd988) at ../../tuxtype_lacki/tuxtype/src/main.c:219

219 TitleScreen(); (gdb) f 0

0 0x00007ffff6ea05a2 in T4K_RunMenu (index=0, return_choice=false, draw_background=0x5555555828a8 , handle_event=0x555555582e0f ,

handle_animations=0x555555583064 <HandleTitleScreenAnimations>, handle_activity=0x555555585427 <handle_activity>) at ../../t4kcommon/src/t4k_menu.c:1058

1058 if (menu->submenu[loc + menu->first_entry]->desc == NULL)

the problem is in 1058 line in src/t4k_menu.c, where "loc" takes the value -1. as "menu->first_entry" =0, then we get "menu->submenu[-1]" and hence segfault. I suggest to add added an enclosing if statement to check if loc is >=0 and < items. This fixes the segfault and allows to show the (easter?) egg as a cursor.

The only change I made is 1058 line in src/t4k_menu.c. All the remaining changes are from pull request #10 by wjt: I am not sure if it is possible (I suppose it is) to make pull request on top of someone else's pull request. For sure I have no idea how to do it.

h01ger commented 10 months ago

I've picked one commit and pushed to master, for the others I dont like the commit messages, can you please improve them?!

matlacki commented 10 months ago

As far as I understand you have picked "fix seg fault" commit (I will improve on commit messages - I did not realize how those appear on github). The one with "c83aa1d"

The two remaining commits together that is bb45ee8 and 3971d27

are 100% equivalent to merge pull request #10 and forget my extra 2 commits. I don't mind changing commit messages (I will try to do it), but an equivalent solution is just to accept pull request #10 .

The commit bb45ee8 undoes one extra change I made in 3971d27 on top of importing files from #10, while testing something.

h01ger commented 10 months ago

On Tue, Nov 14, 2023 at 12:56:02PM -0800, Mateusz Łącki wrote:

As far as I understand you have picked "fix seg fault" commit (I will improve on commit messages - I did not realize how those appear on github).

cool, thanks.

The two remaining commits together that is bb45ee8 and 3971d27

are 100% equivalent to pull request #10 . I don't mind changing commit messages, but an equivalent solution is just to accept pull request #10 .

I think I have merged #10 into t4kcommon's master branch and push.

-- cheers, Holger

⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄

We can send billionaires to space but not kids to fully funded public schools.

matlacki commented 10 months ago

I see that now, then I guess those two my commits are now of no effect. Can just those two be disregarded ?

h01ger commented 10 months ago

On Tue, Nov 14, 2023 at 01:07:07PM -0800, Mateusz Łącki wrote:

I see that now, then I guess those two my commits are now of no effect. Can just those two be disregarded ?

we ca=n just close this pull request and be done.

-- cheers, Holger

⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄

There are no jobs on a dead planet.

matlacki commented 10 months ago

This keeps the one commit that made it into master? If yes, then please close it. I failed miserably at editing my commit messages.

h01ger commented 10 months ago

just closing issues has no effect on any branch whatsover...

h01ger commented 10 months ago

hi,

and closed issues and pull requests are just that: closed. they are still there for future reference.

-- cheers, Holger

⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄

„Ich dachte immer, jeder sei gegen den Krieg, bis ich herausfand, dass es welche gibt, die nicht hingehen müssen.“ (Erich Maria Remarque)