Re: [PATCH] quiltimport: fix backslash expansion in patch subjects

[Sasha Levin <sashal@kernel.org>]

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:
>> 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.

>> 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.

>> 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.

>>    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.

>>    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.

ack!

-- 
Thanks,
Sasha