threads / patch / 45152
Subject: [PATCH 1/3] add git_psprintf helper function
Maxim Moseychuk[PATCH 0/3] Fix l10n
In some places static size buffers can't store formatted string. If it be happen then git die.
Maxim Moseychuk (3): add git_psprintf helper function bisect_next_all: convert xsnprintf to git_psprintf stop_progress_msg: convert xsnprintf to git_psprintf
bisect.c | 9 +++++---- git-compat-util.h | 3 +++ progress.c | 9 +++------ wrapper.c | 19 +++++++++++++++++++ 4 files changed, 30 insertions(+), 10 deletions(-)
-- 2.11.1
Maxim MoseychukThere are a number of places in the code where we call xsnprintf(), with the assumption that the output will fit into the buffer. If the buffer is small, then git die. In many places buffers have compile-time size, but generated string depends from current system locale (gettext)and can have size greater the buffer. Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources - it impossible.
git_psprintf is similar to the standard C sprintf() function but safer, since it calculates the maximum space required and allocates memory to hold the result. The returned string should be freed with free() when no longer needed.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com> --- git-compat-util.h | 3 +++ wrapper.c | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+)
git-compat-util.h, wrapper.c
diff --git a/git-compat-util.h b/git-compat-util.h index 87237b092..fa98705d0 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -878,6 +878,9 @@ static inline size_t xsize_t(off_t len) return (size_t)len; } +__attribute__((format (printf, 1, 2))) +extern char *git_psprintf(const char *fmt, ...); + __attribute__((format (printf, 3, 4))) extern int xsnprintf(char *dst, size_t max, const char *fmt, ...); diff --git a/wrapper.c b/wrapper.c index e7f197996..deee46d2d 100644 --- a/wrapper.c +++ b/wrapper.c @@ -635,6 +635,25 @@ char *xgetcwd(void) return strbuf_detach(&sb, NULL); } +char *git_psprintf(const char *fmt, ...) +{ + va_list ap; + int len; + char *dst; + + va_start(ap, fmt); + len = vsnprintf(NULL, 0, fmt, ap); + va_end(ap); + + dst = xmallocz(len); + + va_start(ap, fmt); + vsprintf(dst, fmt, ap); + va_end(ap); + + return dst; +} + int xsnprintf(char *dst, size_t max, const char *fmt, ...) { va_list ap;
-- 2.11.1
Re: [PATCH 1/3] add git_psprintf helper function
On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:
> There are a number of places in the code where we call > xsnprintf(), with the assumption that the output will fit into > the buffer. If the buffer is small, then git die. > In many places buffers have compile-time size, but generated string > depends from current system locale (gettext)and can have size > greater the buffer. > Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" > on linux sources - it impossible. > > git_psprintf is similar to the standard C sprintf() function > but safer, since it calculates the maximum space required > and allocates memory to hold the result. > The returned string should be freed with free() when no longer needed.
If I understand this correctly, xstrfmt (in strbuf.h) should already do what you need, so you do not need a new function.
Jeff KingRe: [PATCH 1/3] add git_psprintf helper function
On Thu, Feb 16, 2017 at 07:51:14AM -0800, Jonathan Tan wrote:
> On 02/16/2017 03:28 AM, Maxim Moseychuk wrote: > > There are a number of places in the code where we call > > xsnprintf(), with the assumption that the output will fit into > > the buffer. If the buffer is small, then git die. > > In many places buffers have compile-time size, but generated string > > depends from current system locale (gettext)and can have size > > greater the buffer. > > Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" > > on linux sources - it impossible. > > > > git_psprintf is similar to the standard C sprintf() function > > but safer, since it calculates the maximum space required > > and allocates memory to hold the result. > > The returned string should be freed with free() when no longer needed. > > If I understand this correctly, xstrfmt (in strbuf.h) should already do what > you need, so you do not need a new function.
Yes, this is exactly what xstrfmt is for.
-Peff
Maxim Moseychuk[PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
Replace local safe string buffer allocation by git_psprintf.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com> --- progress.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/progress.c b/progress.c index 76a88c573..989d65504 100644 --- a/progress.c +++ b/progress.c @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) *p_progress = NULL; if (progress->last_value != -1) { /* Force the last update */ - char buf[128], *bufp; - size_t len = strlen(msg) + 5; + char *bufp; struct throughput *tp = progress->throughput; - bufp = (len < sizeof(buf)) ? buf : xmallocz(len); if (tp) { unsigned int rate = !tp->avg_misecs ? 0 : tp->avg_bytes / tp->avg_misecs; throughput_string(&tp->display, tp->curr_total, rate); } progress_update = 1; - xsnprintf(bufp, len + 1, ", %s.\n", msg); + bufp = git_psprintf(", %s.\n", msg); display(progress, progress->last_value, bufp); - if (buf != bufp) - free(bufp); + free(bufp); } clear_progress_signal(); if (progress->throughput)
-- 2.11.1
Jeff KingRe: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:
> Replace local safe string buffer allocation by git_psprintf.
Hmm. Is this one actually broken in practice?
I see:
> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
> *p_progress = NULL;
> if (progress->last_value != -1) {
> /* Force the last update */
> - char buf[128], *bufp;
> - size_t len = strlen(msg) + 5;
> + char *bufp;We have a fixed-size buffer here, but...
> struct throughput *tp = progress->throughput; > > - bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
If it's not big enough we allocate a new one. So this works for any size of translated string. It just tries to skip the allocation in the "short" case.
If this were in the normal progress code, I might care more about trying to skip an allocation for performance. But since this is just in stop_progress_msg(), I don't think the complexity is worth it. I'm especially happy to see the magic "5" above go away.
So I think this is worth doing, but the motivation is a simplification, not a bugfix.
> if (tp) {
> unsigned int rate = !tp->avg_misecs ? 0 :
> tp->avg_bytes / tp->avg_misecs;
> throughput_string(&tp->display, tp->curr_total, rate);
> }
> progress_update = 1;
> - xsnprintf(bufp, len + 1, ", %s.\n", msg);
> + bufp = git_psprintf(", %s.\n", msg);
> display(progress, progress->last_value, bufp);
> - if (buf != bufp)
> - free(bufp);
> + free(bufp);This parts looks good (modulo using xstrfmt). It's probably worth renaming "bufp" to the more usual "buf", I would think.
I do wonder if the punctuation here will give translators headaches. E.g., in a RTL language you probably wouldn't want that period at the end. I don't know enough to say, so I'm willing to punt on that for now. But I suspect in the long run we may need to just take the whole translated message, punctuation included, from the caller. There are only two callers that actually provide a string.
-Peff
Maxim Moseychuk[PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf
Git can't run bisect between 2048+ commits if use russian translation. Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
Dummy solution: just increase buffer size but is not safe. Size gettext string is a runtime value.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com> --- bisect.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c index 21bc6daa4..8670cc97a 100644 --- a/bisect.c +++ b/bisect.c @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout) struct commit_list *tried; int reaches = 0, all = 0, nr, steps; const unsigned char *bisect_rev; - char steps_msg[32]; + char *steps_msg; read_bisect_terms(&term_bad, &term_good); if (read_bisect_refs()) @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout) nr = all - reaches - 1; steps = estimate_bisect_steps(all); - xsnprintf(steps_msg, sizeof(steps_msg), - Q_("(roughly %d step)", "(roughly %d steps)", steps), - steps); + + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)", + steps), steps); /* TRANSLATORS: the last %s will be replaced with "(roughly %d steps)" translation */ printf(Q_("Bisecting: %d revision left to test after this %s\n", "Bisecting: %d revisions left to test after this %s\n", nr), nr, steps_msg); + free(steps_msg); return bisect_checkout(bisect_rev, no_checkout); }
-- 2.11.1
Jeff KingRe: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf
On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:
> Git can't run bisect between 2048+ commits if use russian translation. > Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources. > > Dummy solution: just increase buffer size but is not safe. > Size gettext string is a runtime value.
Hmm. I wondered if this used to work (because xsnprintf operated on a fixed-size fmt) and was broken in the translation. And as a consequence, if we needed to be searching for other cases with similar bugs.
But no, in this case the fixed-size buffer was actually introduced during the i18n step from 14dc4899e (i18n: bisect: mark strings for translation, 2016-06-17).
I guess the other type of bug could still exist, though.
> diff --git a/bisect.c b/bisect.c > index 21bc6daa4..8670cc97a 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout) > struct commit_list *tried; > int reaches = 0, all = 0, nr, steps; > const unsigned char *bisect_rev; > - char steps_msg[32]; > + char *steps_msg;
So one solution would be to just bump the "32" higher here. The format comes from the translation, so in practice it only needs to be large enough to fit any of our translations.
That feels pretty hacky, though. In practice the set of translations is contained, but it doesn't have to be (and certainly nobody would notice if a new translation was added with a longer name until a user complained).
> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
>
> nr = all - reaches - 1;
> steps = estimate_bisect_steps(all);
> - xsnprintf(steps_msg, sizeof(steps_msg),
> - Q_("(roughly %d step)", "(roughly %d steps)", steps),
> - steps);
> +
> + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
> + steps), steps);
> /* TRANSLATORS: the last %s will be replaced with
> "(roughly %d steps)" translation */
> printf(Q_("Bisecting: %d revision left to test after this %s\n",
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> + free(steps_msg);I wondered if a viable solution would be to make the whole thing one single translatable string. It would avoid the need for the TRANSLATORS comment. But I guess we have two "Q_" invocations here, and they might need to be handled separately (e.g., "2 revisions", "1 step").
I also wondered if we could just make this into two printf statements ("revisions left to test", followed by "roughly N steps"). But the commit message for 14dc4899e mentions RTL languages. So I think we really do need to build it up block by block and let the translators adjust the ordering.
So I think your solution is the best we can do.
It's probably worth outlining these alternatives in the commit message.
-Peff
Jeff KingRe: [PATCH 0/3] Fix l10n
On Thu, Feb 16, 2017 at 02:28:26PM +0300, Maxim Moseychuk wrote:
> In some places static size buffers can't store formatted string. > If it be happen then git die. > > Maxim Moseychuk (3): > add git_psprintf helper function > bisect_next_all: convert xsnprintf to git_psprintf > stop_progress_msg: convert xsnprintf to git_psprintf
Thanks for providing a series (and I think this is your first series, so welcome!).
The overall goal looks good, and I dropped a few comments in reply to the individual patches.
-Peff