{"thread":{"id":"33653","subject":"[PATCH] pretty: Fix bug in truncation support for %>, %< and %><","startedAt":"2013-04-27T19:43:36Z","lastAt":"2013-04-29T21:29:00Z","messageCount":4,"participants":["Ramsay Jones","Duy Nguyen","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"215737","messageId":"517C2A68.4030802@ramsay1.demon.co.uk","threadId":"33653","inReplyTo":null,"subject":"[PATCH] pretty: Fix bug in truncation support for %>, %< and %><","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-04-27T19:43:36Z","receivedAt":"2013-04-27T19:43:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSome systems experience failures in t4205-*.sh (tests 18-20, 27)\nwhich all relate to the use of truncation with the %< padding\nplaceholder. This capability was added in the commit a7f01c6b\n(\"pretty: support truncating in %>, %< and %><\", 19-04-2013).\n\nThe truncation support was implemented with the assistance of a\nnew strbuf function (strbuf_utf8_replace). This function contains\nthe following code:\n\n strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),\n sb_dst.len, sb_dst.alloc);\n\nUnfortunately, this code is subject to unspecified behaviour. In\nparticular, the order of evaluation of the argument expressions\n(along with the associated side effects) is not specified by the\nC standard. Note that the second argument expression is a call to\nstrbuf_detach() which, as a side effect, sets the 'len' and 'alloc'\nfields of the sb_dst argument to zero. Depending on the order of\nevaluation of the argument expressions to the strbuf_attach call,\nthis can lead to assigning an empty string to 'sb_src'.\n\nIn order to remove the undesired behaviour, we replace the above\nline of code with:\n\n strbuf_swap(sb_src, &sb_dst);\n strbuf_release(&sb_dst);\n\nwhich achieves the desired effect without provoking unspecified\nbehaviour.\n\nSigned-off-by: Ramsay Jones \n---\n utf8.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex b1e1303..0d20e0a 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -463,8 +463,8 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,\n \t\tw += n;\n \t}\n \tstrbuf_setlen(&sb_dst, dst - sb_dst.buf);\n-\tstrbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),\n-\t\t sb_dst.len, sb_dst.alloc);\n+\tstrbuf_swap(sb_src, &sb_dst);\n+\tstrbuf_release(&sb_dst);\n }\n \n int is_encoding_utf8(const char *name)\n-- \n1.8.2\n"},{"id":"215774","messageId":"CACsJy8Dx2tKdQDLw9=i1aM6=uqVBvHYpzq-q+rQE-e7-oJTRcg@mail.gmail.com","threadId":"33653","inReplyTo":"517C2A68.4030802@ramsay1.demon.co.uk","subject":"Re: [PATCH] pretty: Fix bug in truncation support for %>, %< and %><","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-04-27T23:59:04Z","receivedAt":"2013-04-27T23:59:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Apr 28, 2013 at 2:43 AM, Ramsay Jones\n wrote:\n> The truncation support was implemented with the assistance of a\n> new strbuf function (strbuf_utf8_replace). This function contains\n> the following code:\n>\n> strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),\n> sb_dst.len, sb_dst.alloc);\n>\n> Unfortunately, this code is subject to unspecified behaviour. In\n> particular, the order of evaluation of the argument expressions\n> (along with the associated side effects) is not specified by the\n> C standard.\n\nThanks for noticing and fixing it. What compiler did you use by the way?\n--\nDuy\n"},{"id":"215884","messageId":"20130429175551.GA24467@google.com","threadId":"33653","inReplyTo":"517C2A68.4030802@ramsay1.demon.co.uk","subject":"Re: [PATCH] pretty: Fix bug in truncation support for %>, %< and %><","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-29T17:55:51Z","receivedAt":"2013-04-29T17:55:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> Some systems experience failures in t4205-*.sh (tests 18-20, 27)\n> which all relate to the use of truncation with the %< padding\n> placeholder. This capability was added in the commit a7f01c6b\n> (\"pretty: support truncating in %>, %< and %><\", 19-04-2013).\n\nThis is reproducible when running the test suite for 1.8.3-rc0 on some\nDebian test machines (ARM, ia64, powerpc) using gcc 4.6:\n\n https://buildd.debian.org/status/logs.php?pkg=git&ver=1%3A1.8.3~rc0-1\n\n> The truncation support was implemented with the assistance of a\n> new strbuf function (strbuf_utf8_replace). This function contains\n> the following code:\n>\n> strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),\n> sb_dst.len, sb_dst.alloc);\n>\n> Unfortunately, this code is subject to unspecified behaviour. In\n> particular, the order of evaluation of the argument expressions\n> (along with the associated side effects) is not specified by the\n> C standard. Note that the second argument expression is a call to\n> strbuf_detach() which, as a side effect, sets the 'len' and 'alloc'\n> fields of the sb_dst argument to zero.\n\nMakes sense.\n\n[...]\n> In order to remove the undesired behaviour, we replace the above\n> line of code with:\n>\n> strbuf_swap(sb_src, &sb_dst);\n> strbuf_release(&sb_dst);\n>\n> which achieves the desired effect without provoking unspecified\n> behaviour.\n\nNice cleanup. I haven't tested the patch but it looks obviously\ncorrect and I assume you've tested it, so for what it's worth,\n\nReviewed-by: Jonathan Nieder \n"},{"id":"215934","messageId":"517EE61C.5040802@ramsay1.demon.co.uk","threadId":"33653","inReplyTo":"CACsJy8Dx2tKdQDLw9=i1aM6=uqVBvHYpzq-q+rQE-e7-oJTRcg@mail.gmail.com","subject":"Re: [PATCH] pretty: Fix bug in truncation support for %>, %< and %><","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-04-29T21:29:00Z","receivedAt":"2013-04-29T21:29:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Duy Nguyen wrote:\n> On Sun, Apr 28, 2013 at 2:43 AM, Ramsay Jones\n> wrote:\n>> The truncation support was implemented with the assistance of a\n>> new strbuf function (strbuf_utf8_replace). This function contains\n>> the following code:\n>>\n>> strbuf_attach(sb_src, strbuf_detach(&sb_dst, NULL),\n>> sb_dst.len, sb_dst.alloc);\n>>\n>> Unfortunately, this code is subject to unspecified behaviour. In\n>> particular, the order of evaluation of the argument expressions\n>> (along with the associated side effects) is not specified by the\n>> C standard.\n> \n> Thanks for noticing and fixing it.\n\nI didn't notice it; your test noticed it and nagged me to take a look!\n;-)\n\nThe verbose output from the test made it clear that, rather than the\nexpected truncated output, it was returning an empty string. A quick\ntrip to the debugger clearly showed strbuf_detach() clearing the len\nand alloc fields of sb_dst, which was then passed to strbuf_attach().\nSo, I didn't really do much. :-D\n\n> What compiler did you use by the way?\n\nThe compiler was gcc, but that doesn't really matter much.\n\nATB,\nRamsay Jones\n"}]}