Re: [PATCH] quiltimport: fix backslash expansion in patch subjects
Thanks for the review Ben! I'll send a v2 based on your review.
More explanation below:
On Sun, Mar 08, 2026 at 04:56:57PM -0400, Ben Knoble wrote:
Show 7 quoted lines
>> Le 8 mars 2026 à 13:01, Sasha Levin <sashal@kernel.org> a écrit :
>>
>> echo interprets backslash sequences, so a patch with "\0" in its
>> subject has that expanded into a NUL byte, which git commit-tree
>> rejects.
>
>Echo _shouldn’t_ do that without flags like -e depending on your implementation, but I wouldn’t put it past echo ;) hence my preference for printf in scripts.
Right, this is just dash being "funny" and interprets backslash sequences in echo by default.
sasha@laps:~$ dash -c 'echo "hello \0 world"' | xxd 00000000: 6865 6c6c 6f20 0020 776f 726c 640a hello . world. sasha@laps:~$ bash -c 'echo "hello \0 world"' | xxd 00000000: 6865 6c6c 6f20 5c30 2077 6f72 6c64 0a hello \0 world.
>More interestingly, though: do the patch-names or subjects being adjusted below contain such a sequence? They look like user input, so possibly (and we can’t be sure they don’t).
Yes, this was hit in practice. A patch in the Linux kernel stable queue was titled "selftests: tc_actions: don't dump 2MB of \0 to stdout". The literal \0 in the subject caused git-quiltimport to abort mid-import under dash.
>It might be nice to say more about what “rejects” means (errors confusingly? Truncates user input?), but otherwise I expect this is reasonable.
commit_tree_extended() in commit.c does:
if (memchr(msg, '\0', msg_len))
return error("a NUL byte in commit log message not allowed.");So commit-tree errors out, which triggers the "|| exit 4" in the script and aborts the entire quiltimport. Everything after the offending patch in the series is silently lost. I'll add it to the commit message.
Show 23 quoted lines
>> Use printf '%s\n' instead, which doesn't interpret the string.
>>
>> Also quote $tmp_dir to handle paths with spaces.
>>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>> git-quiltimport.sh | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/git-quiltimport.sh b/git-quiltimport.sh
>> index eb34cda409..38302d28c9 100755
>> --- a/git-quiltimport.sh
>> +++ b/git-quiltimport.sh
>> @@ -79,7 +79,7 @@ tmp_info="$tmp_dir/info"
>> # Find the initial commit
>> commit=$(git rev-parse HEAD)
>>
>> -mkdir $tmp_dir || exit 2
>> +mkdir "$tmp_dir" || exit 2
>
>We prefer to leave such “while at it” changes in separate patches, so perhaps a preliminary cleanup “quote variable expansions to handle whitespace” or some such step would help?
>
>(I didn’t look past the context to see if tmp_dir may have whitespace or shell meta characters.)
Will do: I'll split the $tmp_dir quoting into a preliminary cleanup patch.
Show 11 quoted lines
>> while read patch_name level garbage <&3
>> do
>> case "$patch_name" in ''|'#'*) continue;; esac
>> @@ -101,7 +101,7 @@ do
>> echo "$patch_name doesn't exist. Skipping."
>> continue
>> fi
>> - echo $patch_name
>> + printf '%s\n' "$patch_name"
>
>Does this go to commit-tree? Or just protecting the output for the user’s terminal?
Just terminal output. It doesn't feed into commit-tree, but under dash a patch name with backslash sequences would still produce garbled output, so worth fixing alongside.
Show 13 quoted lines
>> git mailinfo $MAILINFO_OPT "$tmp_msg" "$tmp_patch" \
>> <"$QUILT_PATCHES/$patch_name" >"$tmp_info" || exit 3
>> test -s "$tmp_patch" || {
>> @@ -142,14 +142,14 @@ do
>> SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
>> export GIT_AUTHOR_DATE SUBJECT
>> if [ -z "$SUBJECT" ] ; then
>> - SUBJECT=$(echo $patch_name | sed -e 's/.patch$//')
>> + SUBJECT=$(printf '%s' "$patch_name" | sed -e 's/.patch$//')
>
>Interesting. I think POSIX sh supports the ${x#suffix} expansion, which could avoid sed. I think it unlikely the “.” in the RE is intended to match any character rather than a literal dot. But should be done a separate patch and could be left for another series if you wanted (assuming my memory of supported expansions is correct).
>
>Importantly, this does get fed to commit-tree below…Good catch! ${patch_name%.patch} is POSIX and already used throughout git's shell scripts.
Show 17 quoted lines
>> fi
>>
>> if [ -z "$dry_run" ] ; then
>> git apply --index -C1 ${level:+"$level"} "$tmp_patch" &&
>> tree=$(git write-tree) &&
>> - commit=$( { echo "$SUBJECT"; echo; cat "$tmp_msg"; } | git commit-tree $tree -p $commit) &&
>> + commit=$( { printf '%s\n' "$SUBJECT"; echo; cat "$tmp_msg"; } | git commit-tree $tree -p $commit) &&
>> git update-ref -m "quiltimport: $patch_name" HEAD $commit || exit 4
>> fi
>
>… so may need protected as described. Neat.
>
>> done 3<"$QUILT_SERIES"
>> -rm -rf $tmp_dir || exit 5
>> +rm -rf "$tmp_dir" || exit 5
>
>Ditto for cleanup.--
Thanks,
Sasha
