viewer: fix crash when viewing files with PK/ZIP magic bytes#5058
viewer: fix crash when viewing files with PK/ZIP magic bytes#5058ilia-maslakov wants to merge 3 commits intoMidnightCommander:masterfrom
Conversation
github-actions
Bot
added
needs triage
|
/rebase |
mc-butler
force-pushed
the
fix-viewer-zip-crash
branch
from
6221ecb to
74c9f0c
Compare
mc-worker
added
area: mcview
|
@ilia-maslakov I've cleaned up your test and created a |
I vaguely remember we had a related problem some time ago: What I'm not sure about is what the right way to behave here is. I think the idea before was to show an error message. Now the file is silently opened in the raw mode. Is this desired @mc-worker ? Do you know if this is consistent with other viewer behaviors? |
| void mc_editor_plugins_load (void) {} | ||
| int button_get_width (void *b) { (void) b; return 0; } | ||
|
|
||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
I think this should be handled as it is done here: b6012a2 .
| mcview_compute_areas (WView *view) | ||
| { | ||
| (void) view; | ||
| } |
There was a problem hiding this comment.
| mcview_compute_areas (WView *view) | |
| { | |
| (void) view; | |
| } | |
| mcview_compute_areas (MC_UNUSED WView *view) | |
| { | |
| } |
| ssize_t written = write (fd, data, size); | ||
| (void) written; |
There was a problem hiding this comment.
| ssize_t written = write (fd, data, size); | |
| (void) written; | |
| write (fd, data, size); |
It’s better to open the file in raw mode and show at least some result (even if it’s empty) than to interrupt the user with a pop-up error message. This makes the behavior more predictable and less annoying. |
When mcview opens a file with PK/ZIP magic bytes (e.g. PPTX, DOCX, ODS, ODT, JAR), get_compression_type() detects COMPRESSION_ZIP and attempts VFS decompression. If mc_open() on the VFS path fails (fd1 == -1): 1. mcview_close_datasource(view) is called but datasource is DS_NONE (no-op) 2. mcview_show_error() for in-panel viewers calls mcview_set_datasource_string(), setting datasource to DS_STRING 3. No goto/return, so execution falls through to mcview_set_datasource_file() which overwrites datasource to DS_FILE without cleaning up DS_STRING state This corrupts viewer state and causes segfault on subsequent F3 invocation. Fix: when VFS decompression fails, silently fall through to display raw file content. The file is valid and readable, there is no reason to show an error. Closes: MidnightCommander#4760 Signed-off-by: Ilia Maslakov <il.smind@gmail.com>
Add tests verifying that mcview_load correctly handles files with PK/ZIP magic bytes (PPTX, DOCX, ODS, JAR, etc.) when VFS decompression fails. Tests cover: - ZIP-magic file loads as DS_FILE without error - repeated load does not crash (regression for second-F3 segfault) - normal text file loads correctly - nonexistent file returns FALSE - gzip-magic file also loads without error Mark viewer display/lib functions and file_error_message, message, load_file_position as MC_MOCKABLE to enable test overrides. Signed-off-by: Ilia Maslakov <il.smind@gmail.com>
Add extern forward declarations for global linker stub variables (mc_skin_color__cache, mc_editor_plugin_list, we_are_strstrstrbackground) to satisfy -Wmissing-variable-declarations. These variables must remain non-static as they are referenced by name from libinternal.a objects. Signed-off-by: Ilia Maslakov <il.smind@gmail.com>
ilia-maslakov
force-pushed
the
fix-viewer-zip-crash
branch
from
74c9f0c to
2cfa73c
Compare
most definitely NOT. you can't seriously argue that silently doing something different than expected is predictable. if you want to avoid the disruptiveness of a message box, put a red banner above the scrollable area. but then some other mechanism to dismiss the message is required - i guess manually switching to raw mode would do. |
|
I must say I also hate the error popup. When I hit it, I have to find a file that the viewer can open in the neighborhood, hit F3 again, switch to Raw mode, exit the viewer, navigate to the original file and try again. Is there an easier workflow? It seems a much less disrupting way would be displaying a (colored?) error message in the content area, with the possibility to switch to Raw mode right away. |
5 participants
Summary
Root cause
In
mcview_load(), whenget_compression_type()detects a ZIP header andmc_open()on the VFS decompression path fails (fd1 == -1):mcview_close_datasource(view)is called but datasource isDS_NONE(no-op)mcview_show_error()for in-panel viewers callsmcview_set_datasource_string(), setting datasource toDS_STRINGgoto/return, so execution falls through tomcview_set_datasource_file()which overwrites datasource toDS_FILEwithout cleaning upDS_STRINGstateSample content of the test file
The following hex content produces a file that causes
mcviewto segfault when opened:50 4B 03 04 14 00 00 00 08 00This corrupts viewer state and causes segfault on subsequent F3.
Test plan