[PATCH 0/3] Fix l10n

[PATCH 0/3] Fix l10n

[Maxim Moseychuk]

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

[PATCH 1/3] add git_psprintf helper function

[Maxim Moseychuk]

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.

Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
 git-compat-util.h |  3 +++
 wrapper.c         | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

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

[Jonathan Tan]

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.

Re: [PATCH 1/3] add git_psprintf helper function

[Jeff King]

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

[PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf

[Maxim Moseychuk]

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

Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf

[Jeff King]

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

[PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf

[Maxim Moseychuk]

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

Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf

[Jeff King]

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

Re: [PATCH 0/3] Fix l10n

[Jeff King]

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