Skip to content

Fix GString API migration regressions: lack of NUL after strncpy and use after free#5042

Merged
zyv merged 3 commits intoMidnightCommander:masterfrom
peripherium:fix/add-null-termination
Apr 5, 2026
Merged

Fix GString API migration regressions: lack of NUL after strncpy and use after free#5042
zyv merged 3 commits intoMidnightCommander:masterfrom
peripherium:fix/add-null-termination

Conversation

@peripherium
Copy link
Copy Markdown
Contributor

strncpy() was called with text->len, which does not include the terminating NUL byte. As a result, when a shorter filename was copied after a longer one, the global buffer retained trailing characters from the previous value, producing an invalid path.

The fix copies text->len + 1 bytes to include the guaranteed NUL terminator provided by GString.

@github-actions github-actions Bot added needs triage Needs triage by maintainers prio: medium Has the potential to affect progress labels Mar 4, 2026
@github-actions github-actions Bot added this to the Future Releases milestone Mar 4, 2026
@peripherium peripherium force-pushed the fix/add-null-termination branch from 8480e3e to b0d6c82 Compare March 4, 2026 15:42
@mc-worker mc-worker added area: core Issues not related to a specific subsystem and removed needs triage Needs triage by maintainers labels Mar 6, 2026
@mc-worker mc-worker requested review from aborodin and mc-worker and removed request for aborodin March 6, 2026 16:47
@mc-worker
Copy link
Copy Markdown
Contributor

Fixes 260126a.

@mc-worker
Copy link
Copy Markdown
Contributor

/rebase

@mc-worker mc-worker modified the milestones: Future Releases, 4.9.0 Mar 6, 2026
@mc-butler mc-butler force-pushed the fix/add-null-termination branch from b0d6c82 to 5f0017d Compare March 6, 2026 16:50
@zyv
Copy link
Copy Markdown
Member

zyv commented Mar 6, 2026

I wish we had a test for this function, especially when a bug is getting fixed.

@peripherium, would you be up for the task? I have added a test under similar circumstances; you can check this as an example: 7488fc5 .

@KuzinAndrey
Copy link
Copy Markdown
Contributor

I build mc from last commit 9f0be66 and now can't open any tgz archive (after open one). Always get an error:
1
But this fix is help to return to work.

@peripherium peripherium force-pushed the fix/add-null-termination branch from 5f0017d to 8c0ae94 Compare March 14, 2026 11:58
@peripherium
Copy link
Copy Markdown
Contributor Author

Hi,

while adding the requested unit test for exec_make_shell_string(), I ran into a small question regarding the intended testing pattern.

The function was declared static, so I exposed it to the tests using MC_TESTABLE. However, it also relies on the global variable buffer, which is likewise declared static in the same C file.

To make the test build cleanly with and without --enable-tests, I added the following in the forward declarations section in ext.c:

// To prevent compiler warnings from being generated with --enable-tests
MC_TESTABLE
GString *exec_make_shell_string (const char *lc_data, const vfs_path_t *filename_vpath);

// To prevent compiler warnings from being generated without --enable-tests
#if defined(HAVE_TESTS)
extern char buffer[BUF_1K];
#endif

This works for the test, but I wanted to ask if this is the preferred approach in MC, or if there is an established pattern for handling static globals used by testable functions.

If there is a better way to structure this, I’d be happy to adjust the patch.

Thanks.

@mc-worker
Copy link
Copy Markdown
Contributor

@zyv let's merge this PR. Currently, in master, we have broken entering to archives.

@mc-worker
Copy link
Copy Markdown
Contributor

I found another regression in 260126a. The second commit in this PR fixes it.

@zyv let's merge this PR to fix the broken extfs in master.

@zyv
Copy link
Copy Markdown
Member

zyv commented Apr 4, 2026

@zyv let's merge this PR to fix the broken extfs in master.

Sorry, I'll try to have a look today.

@zyv
Copy link
Copy Markdown
Member

zyv commented Apr 4, 2026

/rebase

@mc-butler mc-butler force-pushed the fix/add-null-termination branch from ce2ad8b to 89629c2 Compare April 4, 2026 16:29
peripherium and others added 3 commits April 5, 2026 07:49
Signed-off-by: Manuel Einfalt <einfalt1@proton.me>
Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
extfs_skip_leading_dotslash() returns a const pointer to the substring
of an argument, therefore an argument must not be free'd immediately
after call of extfs_skip_leading_dotslash().

Fixes 260126a.

Signed-off-by: Andrew Borodin <aborodin@vmail.ru>
Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
…ate symbols

Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
@zyv zyv force-pushed the fix/add-null-termination branch from 89629c2 to 31ab988 Compare April 5, 2026 05:50
@zyv zyv changed the title Fix missing NUL termination after strncpy() Fix GString API migration regressions: lack of NUL after strncpy and use after free Apr 5, 2026
Copy link
Copy Markdown
Member

@zyv zyv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, guys, I'm sorry that it has taken me so long.

I have fixed the build on macOS due to duplicate symbols. Otherwise, I find the code good. And I appreciate the test very much. Thank you @peripherium for an excellent contribution!

I will not be adding this to the NEWS, since it's a regression in master after an internal migration.

@zyv zyv merged commit a01afd4 into MidnightCommander:master Apr 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress

Development

Successfully merging this pull request may close these issues.

5 participants