Skip to content

Use POSIX basename() on a copy of the path - #4126

Closed
r3wretrhy wants to merge 1 commit into
rr-debugger:masterfrom
r3wretrhy:fix/posix-basename-4019
Closed

r3wretrhy wants to merge 1 commit into
rr-debugger:masterfrom
r3wretrhy:fix/posix-basename-4019

Conversation

@r3wretrhy

Copy link
Copy Markdown

Fixes #4019.

TraceStream.cc and launch_debugger.cc call basename() on a const char*, which relies on the GNU declaration that glibc puts in <string.h>. Outside glibc that declaration isn't there, which is the TraceStream.cc failure from the #4009 build log.

As agreed in the issue, both call sites now use the POSIX version: include <libgen.h> and hand basename() a copy of the path, since POSIX allows it to modify its argument. In make_trace_dir() the name is now worked out once, before the retry loop.

For ordinary paths the result is the same. I compared old and new on glibc for absolute, relative and bare names and got identical output. They only differ for a path ending in /, which an executable path or the temp script name can't be.

Testing: built rr on Debian 13 (GCC 14, glibc 2.41). rr record created true-0, true-1 and bn_test-0 under _RR_TRACE_DIR, so the trace directory naming and the nonce retry still work. This machine has no hardware perf counters, so recording stops right after that and I couldn't run make check.

@GitMensch you'd offered to do this one. It looked idle, so I put up the small version, but I'm happy to close this if you already have a branch.

TraceStream.cc and launch_debugger.cc called basename() on a const char*,
which only works with the GNU declaration from <string.h>. That
declaration isn't available outside glibc, so include <libgen.h> and
pass the POSIX version a mutable copy instead.

Fixes rr-debugger#4019.

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - thanks for the PR, seems like I haven't checked that in at all...

I've had a quick moment of "why not setting the fixed part once - but the output of trace_save_dir() is cached inside and that's not a tight loop in the first place - still good that you've moved the copy + basename call out of it.

@rocallahan

Copy link
Copy Markdown
Collaborator

Was this produced by AI? If so, please say so according to the AI usage policy:

When submitting issues or PRs, please clearly indicate how LLMs were used. Issues or PRs which appear LLM-generated but carry no such indications may be closed without comment.
When submitting PRs for review, the submitter must be human and take full responsibility for understanding the changes, regardless of how LLMs were used.

@r3wretrhy

Copy link
Copy Markdown
Author

Yes. An AI agent wrote this change, ran the build and checks described above, and wrote the PR description. I missed the policy when it was opened, sorry about that. I've now read the diff and I'm the person responsible for it; if you'd rather not take it on that basis, feel free to close it.

@rocallahan

Copy link
Copy Markdown
Collaborator

I'm not sure it's safe to modify the characters at &module_path[0] ... POSIX basename() might insert a null byte, changing the length of the string, which might confuse std::string. The GNU version is better because it never modifies the string. So we don't want this change.

@rocallahan rocallahan closed this Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix undefined usage of posix basename() passing const char *

3 participants