{"thread":{"id":"45152","subject":"[PATCH 1/3] add git_psprintf helper function","startedAt":"2017-02-16T11:29:55Z","lastAt":"2017-02-16T17:27:39Z","messageCount":9,"participants":["Maxim Moseychuk","Jonathan Tan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"311734","messageId":"20170216112829.18079-2-franchesko.salias.hudro.pedros@gmail.com","threadId":"45152","inReplyTo":"20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com","subject":"[PATCH 1/3] add git_psprintf helper function","fromName":"Maxim Moseychuk","fromEmail":"franchesko.salias.hudro.pedros@gmail.com","sentAt":"2017-02-16T11:28:27Z","receivedAt":"2017-02-16T11:29:55Z","isPatch":true,"sender":{"key":"franchesko.salias.hudro.pedros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3121341?v=4"},"body":"There are a number of places in the code where we call\nxsnprintf(), with the assumption that the output will fit into\nthe buffer. If the buffer is small, then git die.\nIn many places buffers have compile-time size, but generated string\ndepends from current system locale (gettext)and can have size\ngreater the buffer.\nJust run \"LANG=ru_RU.UTF8 git bisect start v4.9 v4.8\"\non linux sources - it impossible.\n\ngit_psprintf is similar to the standard C sprintf() function\nbut safer, since it calculates the maximum space required\nand allocates memory to hold the result.\nThe returned string should be freed with free() when no longer needed.\n\nSigned-off-by: Maxim Moseychuk \n---\n git-compat-util.h | 3 +++\n wrapper.c | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 87237b092..fa98705d0 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -878,6 +878,9 @@ static inline size_t xsize_t(off_t len)\n \treturn (size_t)len;\n }\n \n+__attribute__((format (printf, 1, 2)))\n+extern char *git_psprintf(const char *fmt, ...);\n+\n __attribute__((format (printf, 3, 4)))\n extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex e7f197996..deee46d2d 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -635,6 +635,25 @@ char *xgetcwd(void)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n+char *git_psprintf(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tint len;\n+\tchar *dst;\n+\n+\tva_start(ap, fmt);\n+\tlen = vsnprintf(NULL, 0, fmt, ap);\n+\tva_end(ap);\n+\n+\tdst = xmallocz(len);\n+\n+\tva_start(ap, fmt);\n+\tvsprintf(dst, fmt, ap);\n+\tva_end(ap);\n+\n+\treturn dst;\n+}\n+\n int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n {\n \tva_list ap;\n-- \n2.11.1\n\n"},{"id":"311735","messageId":"20170216112829.18079-4-franchesko.salias.hudro.pedros@gmail.com","threadId":"45152","inReplyTo":"20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com","subject":"[PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf","fromName":"Maxim Moseychuk","fromEmail":"franchesko.salias.hudro.pedros@gmail.com","sentAt":"2017-02-16T11:28:29Z","receivedAt":"2017-02-16T11:30:20Z","isPatch":true,"sender":{"key":"franchesko.salias.hudro.pedros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3121341?v=4"},"body":"Replace local safe string buffer allocation by git_psprintf.\n\nSigned-off-by: Maxim Moseychuk \n---\n progress.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 76a88c573..989d65504 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t*p_progress = NULL;\n \tif (progress->last_value != -1) {\n \t\t/* Force the last update */\n-\t\tchar buf[128], *bufp;\n-\t\tsize_t len = strlen(msg) + 5;\n+\t\tchar *bufp;\n \t\tstruct throughput *tp = progress->throughput;\n \n-\t\tbufp = (len < sizeof(buf)) ? buf : xmallocz(len);\n \t\tif (tp) {\n \t\t\tunsigned int rate = !tp->avg_misecs ? 0 :\n \t\t\t\t\ttp->avg_bytes / tp->avg_misecs;\n \t\t\tthroughput_string(&tp->display, tp->curr_total, rate);\n \t\t}\n \t\tprogress_update = 1;\n-\t\txsnprintf(bufp, len + 1, \", %s.\\n\", msg);\n+\t\tbufp = git_psprintf(\", %s.\\n\", msg);\n \t\tdisplay(progress, progress->last_value, bufp);\n-\t\tif (buf != bufp)\n-\t\t\tfree(bufp);\n+\t\tfree(bufp);\n \t}\n \tclear_progress_signal();\n \tif (progress->throughput)\n-- \n2.11.1\n\n"},{"id":"311736","messageId":"20170216112829.18079-3-franchesko.salias.hudro.pedros@gmail.com","threadId":"45152","inReplyTo":"20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com","subject":"[PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf","fromName":"Maxim Moseychuk","fromEmail":"franchesko.salias.hudro.pedros@gmail.com","sentAt":"2017-02-16T11:28:28Z","receivedAt":"2017-02-16T11:30:32Z","isPatch":true,"sender":{"key":"franchesko.salias.hudro.pedros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3121341?v=4"},"body":"Git can't run bisect between 2048+ commits if use russian translation.\nReproduce \"LANG=ru_RU.UTF8 git bisect start v4.9 v4.8\" on linux sources.\n\nDummy solution: just increase buffer size but is not safe.\nSize gettext string is a runtime value.\n\nSigned-off-by: Maxim Moseychuk \n---\n bisect.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 21bc6daa4..8670cc97a 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n \tconst unsigned char *bisect_rev;\n-\tchar steps_msg[32];\n+\tchar *steps_msg;\n \n \tread_bisect_terms(&term_bad, &term_good);\n \tif (read_bisect_refs())\n@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \n \tnr = all - reaches - 1;\n \tsteps = estimate_bisect_steps(all);\n-\txsnprintf(steps_msg, sizeof(steps_msg),\n-\t\t Q_(\"(roughly %d step)\", \"(roughly %d steps)\", steps),\n-\t\t steps);\n+\n+\tsteps_msg = git_psprintf(Q_(\"(roughly %d step)\", \"(roughly %d steps)\",\n+\t\t steps), steps);\n \t/* TRANSLATORS: the last %s will be replaced with\n \t \"(roughly %d steps)\" translation */\n \tprintf(Q_(\"Bisecting: %d revision left to test after this %s\\n\",\n \t\t \"Bisecting: %d revisions left to test after this %s\\n\",\n \t\t nr), nr, steps_msg);\n+\tfree(steps_msg);\n \n \treturn bisect_checkout(bisect_rev, no_checkout);\n }\n-- \n2.11.1\n\n"},{"id":"311737","messageId":"20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com","threadId":"45152","inReplyTo":null,"subject":"[PATCH 0/3] Fix l10n","fromName":"Maxim Moseychuk","fromEmail":"franchesko.salias.hudro.pedros@gmail.com","sentAt":"2017-02-16T11:28:26Z","receivedAt":"2017-02-16T11:30:41Z","isPatch":true,"sender":{"key":"franchesko.salias.hudro.pedros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3121341?v=4"},"body":"In some places static size buffers can't store formatted string.\nIf it be happen then git die.\n\nMaxim Moseychuk (3):\n add git_psprintf helper function\n bisect_next_all: convert xsnprintf to git_psprintf\n stop_progress_msg: convert xsnprintf to git_psprintf\n\n bisect.c | 9 +++++----\n git-compat-util.h | 3 +++\n progress.c | 9 +++------\n wrapper.c | 19 +++++++++++++++++++\n 4 files changed, 30 insertions(+), 10 deletions(-)\n\n-- \n2.11.1\n\n"},{"id":"311767","messageId":"616a2b08-71f9-d594-d46c-2687bf20ae2b@google.com","threadId":"45152","inReplyTo":"20170216112829.18079-2-franchesko.salias.hudro.pedros@gmail.com","subject":"Re: [PATCH 1/3] add git_psprintf helper function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-16T15:51:14Z","receivedAt":"2017-02-16T15:51:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:\n> There are a number of places in the code where we call\n> xsnprintf(), with the assumption that the output will fit into\n> the buffer. If the buffer is small, then git die.\n> In many places buffers have compile-time size, but generated string\n> depends from current system locale (gettext)and can have size\n> greater the buffer.\n> Just run \"LANG=ru_RU.UTF8 git bisect start v4.9 v4.8\"\n> on linux sources - it impossible.\n>\n> git_psprintf is similar to the standard C sprintf() function\n> but safer, since it calculates the maximum space required\n> and allocates memory to hold the result.\n> The returned string should be freed with free() when no longer needed.\n\nIf I understand this correctly, xstrfmt (in strbuf.h) should already do \nwhat you need, so you do not need a new function.\n"},{"id":"311772","messageId":"20170216170326.sphzwycj3vlgpymm@sigill.intra.peff.net","threadId":"45152","inReplyTo":"616a2b08-71f9-d594-d46c-2687bf20ae2b@google.com","subject":"Re: [PATCH 1/3] add git_psprintf helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T17:03:27Z","receivedAt":"2017-02-16T17:03:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 07:51:14AM -0800, Jonathan Tan wrote:\n\n> On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:\n> > There are a number of places in the code where we call\n> > xsnprintf(), with the assumption that the output will fit into\n> > the buffer. If the buffer is small, then git die.\n> > In many places buffers have compile-time size, but generated string\n> > depends from current system locale (gettext)and can have size\n> > greater the buffer.\n> > Just run \"LANG=ru_RU.UTF8 git bisect start v4.9 v4.8\"\n> > on linux sources - it impossible.\n> > \n> > git_psprintf is similar to the standard C sprintf() function\n> > but safer, since it calculates the maximum space required\n> > and allocates memory to hold the result.\n> > The returned string should be freed with free() when no longer needed.\n> \n> If I understand this correctly, xstrfmt (in strbuf.h) should already do what\n> you need, so you do not need a new function.\n\nYes, this is exactly what xstrfmt is for.\n\n-Peff\n"},{"id":"311778","messageId":"20170216171449.rfn2o2t5ngv7ugxt@sigill.intra.peff.net","threadId":"45152","inReplyTo":"20170216112829.18079-3-franchesko.salias.hudro.pedros@gmail.com","subject":"Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T17:14:50Z","receivedAt":"2017-02-16T17:14:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:\n\n> Git can't run bisect between 2048+ commits if use russian translation.\n> Reproduce \"LANG=ru_RU.UTF8 git bisect start v4.9 v4.8\" on linux sources.\n> \n> Dummy solution: just increase buffer size but is not safe.\n> Size gettext string is a runtime value.\n\nHmm. I wondered if this used to work (because xsnprintf operated on a\nfixed-size fmt) and was broken in the translation. And as a consequence,\nif we needed to be searching for other cases with similar bugs.\n\nBut no, in this case the fixed-size buffer was actually introduced\nduring the i18n step from 14dc4899e (i18n: bisect: mark strings for\ntranslation, 2016-06-17).\n\nI guess the other type of bug could still exist, though.\n\n> diff --git a/bisect.c b/bisect.c\n> index 21bc6daa4..8670cc97a 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n> \tstruct commit_list *tried;\n> \tint reaches = 0, all = 0, nr, steps;\n> \tconst unsigned char *bisect_rev;\n> -\tchar steps_msg[32];\n> +\tchar *steps_msg;\n\nSo one solution would be to just bump the \"32\" higher here. The format\ncomes from the translation, so in practice it only needs to be large\nenough to fit any of our translations.\n\nThat feels pretty hacky, though. In practice the set of translations is\ncontained, but it doesn't have to be (and certainly nobody would notice\nif a new translation was added with a longer name until a user\ncomplained).\n\n> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)\n> \n> \tnr = all - reaches - 1;\n> \tsteps = estimate_bisect_steps(all);\n> -\txsnprintf(steps_msg, sizeof(steps_msg),\n> -\t\t Q_(\"(roughly %d step)\", \"(roughly %d steps)\", steps),\n> -\t\t steps);\n> +\n> +\tsteps_msg = git_psprintf(Q_(\"(roughly %d step)\", \"(roughly %d steps)\",\n> +\t\t steps), steps);\n> \t/* TRANSLATORS: the last %s will be replaced with\n> \t \"(roughly %d steps)\" translation */\n> \tprintf(Q_(\"Bisecting: %d revision left to test after this %s\\n\",\n> \t\t \"Bisecting: %d revisions left to test after this %s\\n\",\n> \t\t nr), nr, steps_msg);\n> +\tfree(steps_msg);\n\nI wondered if a viable solution would be to make the whole thing one\nsingle translatable string. It would avoid the need for the TRANSLATORS\ncomment. But I guess we have two \"Q_\" invocations here, and they might\nneed to be handled separately (e.g., \"2 revisions\", \"1 step\").\n\nI also wondered if we could just make this into two printf statements\n(\"revisions left to test\", followed by \"roughly N steps\"). But the\ncommit message for 14dc4899e mentions RTL languages. So I think we\nreally do need to build it up block by block and let the translators\nadjust the ordering.\n\nSo I think your solution is the best we can do.\n\nIt's probably worth outlining these alternatives in the commit message.\n\n-Peff\n"},{"id":"311779","messageId":"20170216172638.m6obbuca2nqpudeo@sigill.intra.peff.net","threadId":"45152","inReplyTo":"20170216112829.18079-4-franchesko.salias.hudro.pedros@gmail.com","subject":"Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T17:26:38Z","receivedAt":"2017-02-16T17:26:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:\n\n> Replace local safe string buffer allocation by git_psprintf.\n\nHmm. Is this one actually broken in practice?\n\nI see:\n\n> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n> \t*p_progress = NULL;\n> \tif (progress->last_value != -1) {\n> \t\t/* Force the last update */\n> -\t\tchar buf[128], *bufp;\n> -\t\tsize_t len = strlen(msg) + 5;\n> +\t\tchar *bufp;\n\nWe have a fixed-size buffer here, but...\n\n> \t\tstruct throughput *tp = progress->throughput;\n> \n> -\t\tbufp = (len < sizeof(buf)) ? buf : xmallocz(len);\n\nIf it's not big enough we allocate a new one. So this works for any size\nof translated string. It just tries to skip the allocation in the\n\"short\" case.\n\nIf this were in the normal progress code, I might care more about trying\nto skip an allocation for performance. But since this is just in\nstop_progress_msg(), I don't think the complexity is worth it. I'm\nespecially happy to see the magic \"5\" above go away.\n\nSo I think this is worth doing, but the motivation is a simplification,\nnot a bugfix.\n\n> \t\tif (tp) {\n> \t\t\tunsigned int rate = !tp->avg_misecs ? 0 :\n> \t\t\t\t\ttp->avg_bytes / tp->avg_misecs;\n> \t\t\tthroughput_string(&tp->display, tp->curr_total, rate);\n> \t\t}\n> \t\tprogress_update = 1;\n> -\t\txsnprintf(bufp, len + 1, \", %s.\\n\", msg);\n> +\t\tbufp = git_psprintf(\", %s.\\n\", msg);\n> \t\tdisplay(progress, progress->last_value, bufp);\n> -\t\tif (buf != bufp)\n> -\t\t\tfree(bufp);\n> +\t\tfree(bufp);\n\nThis parts looks good (modulo using xstrfmt). It's probably worth\nrenaming \"bufp\" to the more usual \"buf\", I would think.\n\nI do wonder if the punctuation here will give translators headaches.\nE.g., in a RTL language you probably wouldn't want that period at the\nend. I don't know enough to say, so I'm willing to punt on that for now.\nBut I suspect in the long run we may need to just take the whole\ntranslated message, punctuation included, from the caller. There are\nonly two callers that actually provide a string.\n\n-Peff\n"},{"id":"311780","messageId":"20170216172732.jmn7kju76i2suhev@sigill.intra.peff.net","threadId":"45152","inReplyTo":"20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com","subject":"Re: [PATCH 0/3] Fix l10n","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T17:27:33Z","receivedAt":"2017-02-16T17:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 02:28:26PM +0300, Maxim Moseychuk wrote:\n\n> In some places static size buffers can't store formatted string.\n> If it be happen then git die.\n> \n> Maxim Moseychuk (3):\n> add git_psprintf helper function\n> bisect_next_all: convert xsnprintf to git_psprintf\n> stop_progress_msg: convert xsnprintf to git_psprintf\n\nThanks for providing a series (and I think this is your first series, so\nwelcome!).\n\nThe overall goal looks good, and I dropped a few comments in reply to\nthe individual patches.\n\n-Peff\n"}]}