Fix segfault on exit when update check is enabled + small fixes#74
Fix segfault on exit when update check is enabled + small fixes#74sbraz wants to merge 3 commits intoplatomav:masterfrom
Conversation
|
Hello, Thank you for the PR! I had also noticed the thread crash on Windows, when I would execute and terminate the utility fast enough. Generally, MCE will be re-written from scratch at some point, so all of these issues will go away. But, for now, merging this can help. I would like to request one thing, to reverse the whitespace changes only. I hate them, and obviously these won't be there when the project is re-written with good code practices, but for now I'd like to avoid a PR which touches every line of the code more or less, due to padding. |
Without this change, the following happened when piping/redirecting the
output of MCE.py:
```
Error: MC Extractor v1.102.0 crashed, please report the following:
Traceback (most recent call last):
File "/root/MCExtractor/MCE.py", line 1093, in <module>
elif sys_os.startswith('linux') or sys_os == 'darwin' or sys_os.find('bsd') != -1 : sys.stdout.write('\x1b]2;' + mce_title + '\x07')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/colorama/ansitowin32.py", line 47, in write
self.__convertor.write(text)
File "/usr/lib/python3/dist-packages/colorama/ansitowin32.py", line 177, in write
self.write_and_convert(text)
File "/usr/lib/python3/dist-packages/colorama/ansitowin32.py", line 199, in write_and_convert
text = self.convert_osc(text)
^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/colorama/ansitowin32.py", line 272, in convert_osc
winterm.set_title(params[1])
^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'set_title'
```
This looks like a colorama bug, see
tartley/colorama#209.
The workaround is to skip the title change if stdout is not a TTY.
Before this patch, the script could often crash with segfaults on Debian
12/Python 3.11. It apparently when processing small files.
In that case, there was likely a race condition where Python tried to
terminate the update thread while it was already dead.
It was easy to reproduce with:
```sh
for i in {1..100}; do
./MCE.py -skip -exit /lib/firmware/amd-ucode/microcode_amd_fam16h.bin >& /dev/null || echo failed at iteration $i
done
```
GDB showed:
```
Thread 1 "python3" received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
[Thread debugging using libthread_db enabled]
```
Using multiprocessing with daemon=True, there are no more crashes.
Processes are probably easier to kill properly. The result of the
update check is now shared using a queue.
Cool :) I wanted to make pltable and colorama optional for environments where only stdlib packages are available but pltable really is used by a lot of the code. I dropped the whitespace commit. |
|
Yeah, colorama will be removed (no longer maintained) and probably the tabloid-style text too, will see if I do anything similar but as you said, the basic parsing etc takes precedence with these types of refactors. It will be re-written as a project with proper imports, classes, requirements etc. Thank you for the whitespace drop. I tried to execute MCE.py (no parameters) and it crashes at the "main menu" (also something that will go away - anyway): |
|
Sorry, I didn't test on Windows. I can add the |
|
Thank you for the offer. Due to the (terrible) way it's written now, it is not really "__main__" compatible, and such change would indeed require too many changes. There is no point really, it is going to be re-written either way. Btw, the update check functionality with the thread etc has already been removed in the draft of the refactor so, unless the segfault is a big issue, maybe we can leave it as it was. If it is an important issue, I suggest to use -duc and if that does not help either, I prefer to simply delete the update check code altogether. |
Yes, I already do that :) It took me several hours to understand why the script was crashing so I thought I'd share it but if it's going to be rewritten, there is no hurry. Out of curiosity, if you call |
|
Yes no problem, glad to hear that at least -duc is helping. I agree that we can wait until that thing is re-written. I did not test that suggestion from the message (although I suspect it's more complex than that - looks like a generic tip - we are not even freezing anything). The script does not really have a "main" to put this line in, it is effectively some top level code followed by an enormous "for" loop. Trust me, it is terrible haha. 😅 |
Hi,
Thanks for maintaining this tool. I ran into a few issues lately so I'm sharing my fixes:
Make MCE.py executable
Fix crash when stdout is not a TTY on Linux
Without this change, the following happened when piping/redirecting the
output of MCE.py:
This looks like a colorama bug, see
Should init() be wrapping stdout on Linux? tartley/colorama#209.
The workaround is to skip the title change if stdout is not a TTY.
Fix segfault on exit when update check is enabled
Before this patch, the script could often crash with segfaults on Debian
12/Python 3.11. It apparently when processing small files.
In that case, there was likely a race condition where Python tried to
terminate the update thread while it was already dead.
It was easy to reproduce with:
GDB showed:
Using multiprocessing with daemon=True, there are no more crashes.
Processes are probably easier to kill properly. The result of the
update check is now shared using a queue.