Eric Sunshine [off-list ref] writes:
This new example raises the question about what happens if the
argument to --reroll-count contains characters which don't belong in
pathnames. For instance, what happens if `--reroll-count=1/2` is
specified? Most likely, it will fail trying to write the
"v1/2-whatever.patch" file to a nonexistent directory named "v1".
quoted
diff --git a/log-tree.c b/log-tree.c
@@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
+ if (info->reroll_count)
+ strbuf_addf(filename, "v%s-", info->reroll_count);
strbuf_addf(filename, "%04d-%s", nr, subject);
To protect against that problem, you may need to call
format_sanitized_subject() manually after formatting "v%s-". (I'm just
looking at this code for the first time, so I could be hopelessly
wrong. There may be a better way to fix it.)
This kind of discovery of what I and others missed is why I love to
see reviews on this list ;-)
Yes, slash is of course very problematic, but what we've been doing
to the patch filenames was to ensure that they will be free of $IFS
whitespaces and shell glob special characters as well, and we should
treat the "reroll count" just like the other end-user controlled
input, i.e. the title of the patch, and sanitize it the same way.
So I am pretty sure format_sanitized_subject() is the right way to
go.
On Sun, Mar 21, 2021 at 1:45 AM Junio C Hamano [off-list ref] wrote:
Eric Sunshine [off-list ref] writes:
quoted
To protect against that problem, you may need to call
format_sanitized_subject() manually after formatting "v%s-". (I'm just
looking at this code for the first time, so I could be hopelessly
wrong. There may be a better way to fix it.)
Yes, slash is of course very problematic, but what we've been doing
to the patch filenames was to ensure that they will be free of $IFS
whitespaces and shell glob special characters as well, and we should
treat the "reroll count" just like the other end-user controlled
input, i.e. the title of the patch, and sanitize it the same way.
So I am pretty sure format_sanitized_subject() is the right way to
go.
The pathname sanitization would also deserve a test.
Denton's seemingly simple feature request[1] has turned out to be
quite a little project.
[1]: https://github.com/gitgitgadget/git/issues/882
On Sun, Mar 21, 2021 at 01:54:15AM -0400, Eric Sunshine wrote:
On Sun, Mar 21, 2021 at 1:45 AM Junio C Hamano [off-list ref] wrote:
quoted
Eric Sunshine [off-list ref] writes:
quoted
To protect against that problem, you may need to call
format_sanitized_subject() manually after formatting "v%s-". (I'm just
looking at this code for the first time, so I could be hopelessly
wrong. There may be a better way to fix it.)
Yes, slash is of course very problematic, but what we've been doing
to the patch filenames was to ensure that they will be free of $IFS
whitespaces and shell glob special characters as well, and we should
treat the "reroll count" just like the other end-user controlled
input, i.e. the title of the patch, and sanitize it the same way.
So I am pretty sure format_sanitized_subject() is the right way to
go.
The pathname sanitization would also deserve a test.
Denton's seemingly simple feature request[1] has turned out to be
quite a little project.
Sorry I've been quite busy the past couple of weeks so I haven't had the
bandwidth to review the patches as they've come up. Thanks for
implementing my feature request, ZheNing. And thanks for the careful
reviews, Eric.
[1]: https://github.com/gitgitgadget/git/issues/882