Skip to content

Don't escape ! in r_str_escape_sh on Unix ##util - #27287

Merged
trufae merged 1 commit into
radareorg:masterfrom
Thanatos9404:file-copy-in-c
Oct 31, 2026
Merged

trufae merged 1 commit into
radareorg:masterfrom
Thanatos9404:file-copy-in-c

Conversation

@Thanatos9404

@Thanatos9404 Thanatos9404 commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor
  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some lines in the book (optional)

Description

On Unix r_str_escape_sh escaped ! as \!. Its callers put the result inside double quotes and run it through a non-interactive shell, which does no history expansion and keeps the backslash there, so r_file_copy (cp -f), the cross-device mv fallback in r_file_move and the other callers could not handle names containing !.

This drops the ! case on Unix. $, backticks, " and \ are still escaped, and Windows is unchanged.

test_r_str_escape_sh now expects ! to stay as is, and the new test_file_copy copies a file named copy_src!$`"\ through r_file_copy. On Linux both fail on master (cp: cannot stat 'copy_src\!$`"\') and pass with this change. sys/lint.py reports nothing.

Native copying and the 4096-byte r_sys_cmdf limit are left for a separate change.

trufae commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator

I'd hold off merging this as-is. I reproduced three regressions against cp -f:

  • Directory destinations fail with EISDIR. r_file_copy("src", "existing_dir") used to create existing_dir/src. There is still a caller relying on this in libr/util/rvc_rvc.c:file_copyp(). Please keep that behavior and cover it.
  • The self-copy check happens after the EACCES fallback has already unlinked the destination. With a mode-0400 source, copying src to ./src returns true and replaces its inode. cp -f refuses and leaves it alone. Check identity before unlinking too. Please add a read-only alias/hardlink case; the current self-copy test uses a writable source.
  • Sparse files lose their holes. A 64 MiB sparse source used zero allocated blocks after cp -f, but about 64 MiB after this helper. Large disk images copied through prj.files can now consume much more space and hit ENOSPC. Please preserve holes if we keep the native copy.

These checks used the exact helper in a small Linux harness, with DAC override disabled for the permission case. I haven't run the full testsuite.

Do we need the native-copy rewrite in this fix? Could we first remove the Unix ! escaping from r_str_escape_sh? Noninteractive sh does not need it, and that would also fix the nearby r_file_move fallback with a much smaller patch. The command-length limit and native copying could then be handled separately.

@Thanatos9404 Thanatos9404 changed the title Copy files in C so names with ! work in r_file_copy ##util Don't escape ! in r_str_escape_sh on Unix ##util Oct 31, 2026
@Thanatos9404

Copy link
Copy Markdown
Contributor Author

Thanks for testing these. You're right that the native copy isn't needed for this bug, so I replaced it with your suggestion: the PR now only drops the ! escape from r_str_escape_sh on Unix, which also fixes the r_file_move fallback and the other callers that build sh -c commands. The new test copies copy_src!$`"\ through r_file_copy and fails on master; native copying and the command-length limit can be a separate change.

@trufae
trufae merged commit 00c9778 into radareorg:master Oct 31, 2026
50 checks passed
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.

2 participants