{"thread":{"id":"41955","subject":"git segfaults on older Solaris releases","startedAt":"2016-04-07T18:18:49Z","lastAt":"2016-04-12T10:21:24Z","messageCount":17,"participants":["Tom G. Christensen","Junio C Hamano","David Turner","Jeff King","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282866","messageId":"5706A489.7070101@jupiterrise.com","threadId":"41955","inReplyTo":null,"subject":"git segfaults on older Solaris releases","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-07T18:18:49Z","receivedAt":"2016-04-07T18:18:49Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"Hello,\n\nWhile working on an update to the git packages in tgcware(1) I ran into \nsegfaults when running the testsuite.\n\nHere's what it looks like on Solaris 7/SPARC:\n\nCore was generated by \n`/export/home/tgc/buildpkg/git/src/git-upstream/git update-index \nshould-be-empty'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0xfee81ef4 in _doprnt () from /usr/lib/libc.so.1\n(gdb) bt\n#0 0xfee81ef4 in _doprnt () from /usr/lib/libc.so.1\n#1 0xfee83ce4 in vsnprintf () from /usr/lib/libc.so.1\n#2 0x00138dbc in strbuf_vaddf (sb=0xffbedd24, fmt=0x1af7b8 \"%.*s%s\", \nap=0xffbedde0) at strbuf.c:279\n#3 0x00139f78 in xstrvfmt (fmt=0x1af7b8 \"%.*s%s\", ap=0xffbedde0) at \nstrbuf.c:698\n#4 0x00139fb4 in xstrfmt (fmt=0x1af7b8 \"%.*s%s\") at strbuf.c:708\n#5 0x0012a0ec in prefix_path_gently (prefix=0x0, len=0, \nremaining_prefix=0x0, path=) at setup.c:103\n#6 0x0012a2f0 in prefix_path (prefix=0x0, len=0, path=0xffbee7fc \n\"should-be-empty\") at setup.c:116\n#7 0x00098464 in cmd_update_index (argc=2, argv=, \nprefix=0x0) at builtin/update-index.c:1042\n#8 0x00025900 in run_builtin (argv=0xffbee630, argc=2, p=0x1c9adc \n) at git.c:346\n#9 handle_builtin (argc=2, argv=0xffbee630) at git.c:536\n#10 0x00025bec in run_argv (argv=0xffbee5c4, argcp=0xffbee60c) at git.c:582\n#11 main (argc=2, av=) at git.c:690\n(gdb)\n\n\nThe reason for the crash is simple, a null value was passed to the 's' \nformat for the *printf family of functions.\nTo verify I modified git.c:run_builtin() so it would assign \"\" to prefix \nif NULL just before the status = p->fn(..) call.\nThis allowed t0000-basic.sh to pass where before it would fail because \ngit segfaulted in multiple tests.\n\nPassing a null value to the 's' format is explicitly documented as \ngiving undefined results on Solaris, even on Solaris 11(2).\nIt happens that Solaris 8 and later will tolerate this without crashing, \nthough I suspect at least for Solaris 8 and 9 it might require a certain \npatchlevel to do so. Earlier releases will just segfault as shown above.\n\nI bisected it on Solaris 2.6 and found that 75faa45 was the commit that \ncaused this problem to appear. The 2.6.x releases build and run fine.\n\nI know of course that Solaris < 8 is not terribly interesting as a \nportability target so I understand if you're unwilling to fix this as it \nseems it might be a somewhat invasive change.\n\n-tgc\n\n1) http://jupiterrise.com/tgcware/tgcware.solaris.html\n2) http://docs.oracle.com/cd/E23824_01/html/821-1465/printf-3c.html\n"},{"id":"282867","messageId":"xmqqoa9lz2uw.fsf@gitster.mtv.corp.google.com","threadId":"41955","inReplyTo":"5706A489.7070101@jupiterrise.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T18:32:39Z","receivedAt":"2016-04-07T18:32:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tom G. Christensen\" writes:\n\n> The reason for the crash is simple, a null value was passed to the 's'\n> format for the *printf family of functions.\n> ...\n> Passing a null value to the 's' format is explicitly documented as\n> giving undefined results on Solaris, even on Solaris 11(2).\n\nDo you mean\n\n\t*printf(\"...%.*s...\", ..., 0, NULL, ...)\n\ni.e. you saw a NULL passed only when we use %.*s with width=0?\n"},{"id":"282869","messageId":"xmqqk2k9z20p.fsf@gitster.mtv.corp.google.com","threadId":"41955","inReplyTo":"xmqqoa9lz2uw.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T18:50:46Z","receivedAt":"2016-04-07T18:50:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Tom G. Christensen\" writes:\n>\n>> The reason for the crash is simple, a null value was passed to the 's'\n>> format for the *printf family of functions.\n>> ...\n>> Passing a null value to the 's' format is explicitly documented as\n>> giving undefined results on Solaris, even on Solaris 11(2).\n>\n> Do you mean\n>\n> \t*printf(\"...%.*s...\", ..., 0, NULL, ...)\n>\n> i.e. you saw a NULL passed only when we use %.*s with width=0?\n\nSo, I've looked at places where we use \"%.*s\" with \"prefix\" nearby,\nand it seems that this is the only place.\n\nThe \"prefix\" being a NULL is a perfectly valid state throughout the\nsystem and means a different thing than it being an empty string, so\nit is valid for callers of prefix_path() and prefix_path_gently() to\npass prefix=NULL as long as they pass len=0.\n\nSo perhaps this is all we need to fix your box.\n\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3439ec6..b6c8aab 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\t\treturn NULL;\n \t\t}\n \t} else {\n-\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix, path);\n+\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix ? prefix : \"\", path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n \t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n"},{"id":"282870","messageId":"1460055400.5540.3.camel@twopensource.com","threadId":"41955","inReplyTo":"xmqqk2k9z20p.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-07T18:56:40Z","receivedAt":"2016-04-07T18:56:40Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2016-04-07 at 11:50 -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > \"Tom G. Christensen\" writes:\n> > \n> > > The reason for the crash is simple, a null value was passed to\n> > > the 's'\n> > > format for the *printf family of functions.\n> > > ...\n> > > Passing a null value to the 's' format is explicitly documented\n> > > as\n> > > giving undefined results on Solaris, even on Solaris 11(2).\n> > \n> > Do you mean\n> > \n> > \t*printf(\"...%.*s...\", ..., 0, NULL, ...)\n> > \n> > i.e. you saw a NULL passed only when we use %.*s with width=0?\n> \n> So, I've looked at places where we use \"%.*s\" with \"prefix\" nearby,\n> and it seems that this is the only place.\n> \n> The \"prefix\" being a NULL is a perfectly valid state throughout the\n> system and means a different thing than it being an empty string, so\n> it is valid for callers of prefix_path() and prefix_path_gently() to\n> pass prefix=NULL as long as they pass len=0.\n\nTIOLI: The code below does not reflect the \"as long as they pass len=0\"\ncondition. Maybe add an assert for that? \n"},{"id":"282871","messageId":"5706ADC1.7030709@jupiterrise.com","threadId":"41955","inReplyTo":"xmqqoa9lz2uw.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-07T18:58:09Z","receivedAt":"2016-04-07T18:58:09Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 07/04/16 20:32, Junio C Hamano wrote:\n> \"Tom G. Christensen\" writes:\n>\n>> The reason for the crash is simple, a null value was passed to the 's'\n>> format for the *printf family of functions.\n>> ...\n>> Passing a null value to the 's' format is explicitly documented as\n>> giving undefined results on Solaris, even on Solaris 11(2).\n>\n> Do you mean\n>\n> \t*printf(\"...%.*s...\", ..., 0, NULL, ...)\n>\n> i.e. you saw a NULL passed only when we use %.*s with width=0?\n>\n\nMaybe? Not sure what you're asking exactly.\n\nI'm seing what is in the backtrace from gdb and that is prefix is NULL \n(0x0) which ends up being printed using some variant of '%s' after going \nthrough the various wrappers.\n\nI hacked around it in run_builtin() as a proof and have also made some \nexperiments with working around it in setup_git_directory_gently() which \ngot me a bit further but it looks like there are places that do \nif(prefix) which now does not behave as expected because prefix is not NULL.\n\n-tgc\n"},{"id":"282900","messageId":"20160407190709.GC4478@sigill.intra.peff.net","threadId":"41955","inReplyTo":"xmqqk2k9z20p.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T19:07:10Z","receivedAt":"2016-04-07T19:07:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 11:50:46AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > \"Tom G. Christensen\" writes:\n> >\n> >> The reason for the crash is simple, a null value was passed to the 's'\n> >> format for the *printf family of functions.\n> >> ...\n> >> Passing a null value to the 's' format is explicitly documented as\n> >> giving undefined results on Solaris, even on Solaris 11(2).\n\nThanks, TIL (though it is not really surprising, I guess, since some\nmemcpy implementations have the same problem).\n\n> So, I've looked at places where we use \"%.*s\" with \"prefix\" nearby,\n> and it seems that this is the only place.\n\nThank you for digging; I obviously didn't think about this issue at all\nwhen doing the mass conversions recently.\n\n> The \"prefix\" being a NULL is a perfectly valid state throughout the\n> system and means a different thing than it being an empty string, so\n> it is valid for callers of prefix_path() and prefix_path_gently() to\n> pass prefix=NULL as long as they pass len=0.\n> \n> So perhaps this is all we need to fix your box.\n> \n> setup.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/setup.c b/setup.c\n> index 3439ec6..b6c8aab 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,\n> \t\t\treturn NULL;\n> \t\t}\n> \t} else {\n> -\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix, path);\n> +\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix ? prefix : \"\", path);\n> \t\tif (remaining_prefix)\n> \t\t\t*remaining_prefix = len;\n> \t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n\nThe original pre-75faa45ae0230b321bf72027b2274315d7e14e34 version\nchecked \"if (len)\", but I think this should be equally right.\n\n-Peff\n"},{"id":"282901","messageId":"xmqq8u0pyzu6.fsf@gitster.mtv.corp.google.com","threadId":"41955","inReplyTo":"20160407190709.GC4478@sigill.intra.peff.net","subject":"Re: git segfaults on older Solaris releases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T19:37:53Z","receivedAt":"2016-04-07T19:37:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> So perhaps this is all we need to fix your box.\n>> \n>> setup.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/setup.c b/setup.c\n>> index 3439ec6..b6c8aab 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,\n>> \t\t\treturn NULL;\n>> \t\t}\n>> \t} else {\n>> -\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix, path);\n>> +\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix ? prefix : \"\", path);\n>> \t\tif (remaining_prefix)\n>> \t\t\t*remaining_prefix = len;\n>> \t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n>\n> The original pre-75faa45ae0230b321bf72027b2274315d7e14e34 version\n> checked \"if (len)\", but I think this should be equally right.\n\nThat's a good point. The original had\n\n- sanitized = xmalloc(len + strlen(path) + 1);\n- if (len)\n- memcpy(sanitized, prefix, len);\n- strcpy(sanitized + len, path);\n+ sanitized = xstrfmt(\"%.*s%s\", len, prefix, path);\n\nand for a brief moment I was wondering why the strlen() did not\nbarf, until I realized that was on path not prefix.\n\n-- >8 --\nSubject: setup.c: do not feed NULL to \"%.*s\" even with the precision 0\n\nA recent update 75faa45a (replace trivial malloc + sprintf / strcpy\ncalls with xstrfmt, 2015-09-24) rewrote\n\n\tprepare an empty buffer\n\tif (len)\n \tappend the first len bytes of \"prefix\" to the buffer\n\tappend \"path\" to the buffer\n\nthat computed \"path\", optionally prefixed by \"prefix\", into\n\n\txstrfmt(\"%.*s%s\", len, prefix, path);\n\nHowever, passing a NULL pointer to the printf(3) family of functions\nto format it with %s conversion, even with the precision 0, i.e.\n\n\txstrfmt(\"%.*s\", 0, NULL)\n\nyields undefined results, at least on some platforms. \n\nAvoid this problem by substituting prefix with \"\" when len==0, as\nprefix can legally be NULL in that case. This would mimick the\nintent of the original code better.\n\nReported-by: \"Tom G. Christensen\" \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3439ec6..b4a92fe 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\t\treturn NULL;\n \t\t}\n \t} else {\n-\t\tsanitized = xstrfmt(\"%.*s%s\", len, prefix, path);\n+\t\tsanitized = xstrfmt(\"%.*s%s\", len, len ? prefix : \"\", path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n \t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n"},{"id":"282905","messageId":"5706C0D4.9030707@jupiterrise.com","threadId":"41955","inReplyTo":"xmqqk2k9z20p.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-07T20:19:32Z","receivedAt":"2016-04-07T20:19:32Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 07/04/16 20:50, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> So perhaps this is all we need to fix your box.\n>\n> setup.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n\nYes that seems to work very well.\n\nI applied this patch to 2.8.0 and have completed a testsuite run on \nSolaris 2.6/x86 with only a few unrelated problems.\nI will continue testing on the other Solaris < 10 releases but I do not \nexpect any difference in the outcome.\n\n-tgc\n"},{"id":"282906","messageId":"20160407202448.GA7705@sigill.intra.peff.net","threadId":"41955","inReplyTo":"xmqq8u0pyzu6.fsf@gitster.mtv.corp.google.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-07T20:24:48Z","receivedAt":"2016-04-07T20:24:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 07, 2016 at 12:37:53PM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: setup.c: do not feed NULL to \"%.*s\" even with the precision 0\n> \n> A recent update 75faa45a (replace trivial malloc + sprintf / strcpy\n> calls with xstrfmt, 2015-09-24) rewrote\n> \n> \tprepare an empty buffer\n> \tif (len)\n> \tappend the first len bytes of \"prefix\" to the buffer\n> \tappend \"path\" to the buffer\n> \n> that computed \"path\", optionally prefixed by \"prefix\", into\n> \n> \txstrfmt(\"%.*s%s\", len, prefix, path);\n> \n> However, passing a NULL pointer to the printf(3) family of functions\n> to format it with %s conversion, even with the precision 0, i.e.\n> \n> \txstrfmt(\"%.*s\", 0, NULL)\n> \n> yields undefined results, at least on some platforms. \n> \n> Avoid this problem by substituting prefix with \"\" when len==0, as\n> prefix can legally be NULL in that case. This would mimick the\n> intent of the original code better.\n> \n> Reported-by: \"Tom G. Christensen\" \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n\nNicely explained.\n\nAcked-by: Jeff King \n\nThanks.\n\n-Peff\n"},{"id":"283009","messageId":"5708A90E.1050705@jupiterrise.com","threadId":"41955","inReplyTo":"5706C0D4.9030707@jupiterrise.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-09T07:02:38Z","receivedAt":"2016-04-09T07:02:38Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 07/04/16 22:19, Tom G. Christensen wrote:\n> On 07/04/16 20:50, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> So perhaps this is all we need to fix your box.\n>>\n>> setup.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>\n> I applied this patch to 2.8.0 and have completed a testsuite run on\n> Solaris 2.6/x86 with only a few unrelated problems.\n> I will continue testing on the other Solaris < 10 releases but I do not\n> expect any difference in the outcome.\n>\n\nI've finished testing 2.8.1 and I found one more case where a null is \nbeing printed and causing a segfault. This happens even on Solaris 8 and 9.\nThe failling test is t3200.63.\n\nHere is the backtrace from a Solaris 8/SPARC machine:\n\n(gdb) core trash directory.t3200-branch/core\n[New LWP 1]\n[Thread debugging using libthread_db enabled]\n[New Thread 1 (LWP 1)]\nCore was generated by `/export/home/tgc/buildpkg/git/src/git-2.8.1/git \nbranch --unset-upstream'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0xfecb32cc in strlen () from /usr/lib/libc.so.1\n(gdb) bt\n#0 0xfecb32cc in strlen () from /usr/lib/libc.so.1\n#1 0xfed06508 in _doprnt () from /usr/lib/libc.so.1\n#2 0xfed08690 in vfprintf () from /usr/lib/libc.so.1\n#3 0x001487bc in vreportf (prefix=, err=, \nparams=0xffbfe408) at usage.c:23\n#4 0x0014881c in die_builtin (err=0x198f90 \"Could not set '%s' to \n'%s'\", params=0xffbfe408) at usage.c:35\n#5 0x00148934 in die (err=0x198f90 \"Could not set '%s' to '%s'\") at \nusage.c:108\n#6 0x000af1b0 in git_config_set_multivar_in_file (value=0x0, \nkey=0x1ecca0 \"branch.master.remote\",\n config_filename=, value_regex=, \nmulti_replace=) at config.c:2226\n#7 git_config_set_multivar_in_file (config_filename=0x0, key=0x1ecca0 \n\"branch.master.remote\", value=0x0,\n value_regex=0x0, multi_replace=1) at config.c:2220\n#8 0x0003aa6c in cmd_branch (argc=0, argv=0xffbfec00, prefix=) at builtin/branch.c:793\n#9 0x000255e8 in run_builtin (argv=0xffbfec00, argc=2, p=0x1c365c \n) at git.c:353\n#10 handle_builtin (argc=2, argv=0xffbfec00) at git.c:540\n#11 0x00168ecc in run_argv (argv=0xffbfeb30, argcp=0xffbfebdc) at git.c:594\n#12 main (argc=2, av=) at git.c:701\n(gdb)\n\n\n-tgc\n"},{"id":"283025","messageId":"20160409173904.GA5127@sigill.intra.peff.net","threadId":"41955","inReplyTo":"5708A90E.1050705@jupiterrise.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T17:39:05Z","receivedAt":"2016-04-09T17:39:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 09, 2016 at 09:02:38AM +0200, Tom G. Christensen wrote:\n\n> I've finished testing 2.8.1 and I found one more case where a null is being\n> printed and causing a segfault. This happens even on Solaris 8 and 9.\n> The failling test is t3200.63.\n\nOh good, this one wasn't me. :)\n\nIt's just a normal \"oops, we feed NULL and nobody on glibc noticed\nbecause it silently replaced it with \"(null)\" case. I did find a few\nother oddities while fixing it, though. +cc Patrick, who worked in this\narea most recently.\n\n [1/3]: config: lower-case first word of error strings\n [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors\n [3/3]: git_config_set_multivar_in_file: handle \"unset\" errors\n\nI think we may want some additional improvements. While doing 1/3, I\nnoticed that many of these error messages could stand to be marked for\ntranslation. As other people are already looking at mass-conversion,\nI stopped short of doing it here (and merely contented myself with\nthrowing a conflict into their patches ;) ).\n\nThe other thing is that 2/3 notices the error return from the\nconfig-setting functions is weird. It's sometimes negative and sometimes\npositive. I fixed this caller, but I think it's possible for the\nnegative values to leak into our exit codes:\n\n $ touch .git/config\n $ git config foo.bar baz\n error: could not lock config file .git/config: File exists\n $ echo $?\n 255\n\nI seem to recall some systems having trouble with negative error codes,\nso we may want to make that more consistent.\n\n-Peff\n"},{"id":"283026","messageId":"20160409174230.GA11873@sigill.intra.peff.net","threadId":"41955","inReplyTo":"20160409173904.GA5127@sigill.intra.peff.net","subject":"[PATCH 1/3] config: lower-case first word of error strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T17:42:31Z","receivedAt":"2016-04-09T17:42:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This follows our usual style (both throughout git, and\nthroughout the rest of this file).\n\nThis covers the whole file, but note that I left the capitalization in\nthe multi-sentence:\n\n error: malformed value...\n error: Must be one of ...\n\nbecause it helps make it clear that we are starting a new sentence in\nthe second one.\n\nSigned-off-by: Jeff King \n---\n config.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 8f66519..6b81931 100644\n--- a/config.c\n+++ b/config.c\n@@ -108,7 +108,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \n \texpanded = expand_user_path(path);\n \tif (!expanded)\n-\t\treturn error(\"Could not expand include path '%s'\", path);\n+\t\treturn error(\"could not expand include path '%s'\", path);\n \tpath = expanded;\n \n \t/*\n@@ -950,7 +950,7 @@ static int git_default_branch_config(const char *var, const char *value)\n \t\telse if (!strcmp(value, \"always\"))\n \t\t\tautorebase = AUTOREBASE_ALWAYS;\n \t\telse\n-\t\t\treturn error(\"Malformed value for %s\", var);\n+\t\t\treturn error(\"malformed value for %s\", var);\n \t\treturn 0;\n \t}\n \n@@ -976,7 +976,7 @@ static int git_default_push_config(const char *var, const char *value)\n \t\telse if (!strcmp(value, \"current\"))\n \t\t\tpush_default = PUSH_DEFAULT_CURRENT;\n \t\telse {\n-\t\t\terror(\"Malformed value for %s: %s\", var, value);\n+\t\t\terror(\"malformed value for %s: %s\", var, value);\n \t\t\treturn error(\"Must be one of nothing, matching, simple, \"\n \t\t\t\t \"upstream or current.\");\n \t\t}\n@@ -2223,7 +2223,7 @@ void git_config_set_multivar_in_file(const char *config_filename,\n {\n \tif (git_config_set_multivar_in_file_gently(config_filename, key, value,\n \t\t\t\t\t\t value_regex, multi_replace) < 0)\n-\t\tdie(_(\"Could not set '%s' to '%s'\"), key, value);\n+\t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n }\n \n int git_config_set_multivar_gently(const char *key, const char *value,\n@@ -2404,7 +2404,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n #undef config_error_nonbool\n int config_error_nonbool(const char *var)\n {\n-\treturn error(\"Missing value for '%s'\", var);\n+\treturn error(\"missing value for '%s'\", var);\n }\n \n int parse_config_key(const char *var,\n-- \n2.8.1.245.g18e0f5c\n"},{"id":"283027","messageId":"20160409174253.GB11873@sigill.intra.peff.net","threadId":"41955","inReplyTo":"20160409173904.GA5127@sigill.intra.peff.net","subject":"[PATCH 2/3] git_config_set_multivar_in_file: all non-zero returns are errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T17:42:54Z","receivedAt":"2016-04-09T17:42:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function is just a thin wrapper for the \"_gently\" form\nof the function. But the gently form is designed to feed\nbuiltin/config.c, which passes our return code directly to\nits exit status, and thus uses positive error values for\nsome cases. We check only negative values, meaning we would\nfail to die in some cases (e.g., a malformed key).\n\nThis may or may not be triggerable in practice; we tend to\nuse this non-gentle form only when setting internal\nvariables, which would not have malformed keys.\n\nSigned-off-by: Jeff King \n---\n config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 6b81931..d446315 100644\n--- a/config.c\n+++ b/config.c\n@@ -2222,7 +2222,7 @@ void git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t const char *value_regex, int multi_replace)\n {\n \tif (git_config_set_multivar_in_file_gently(config_filename, key, value,\n-\t\t\t\t\t\t value_regex, multi_replace) < 0)\n+\t\t\t\t\t\t value_regex, multi_replace))\n \t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n }\n \n-- \n2.8.1.245.g18e0f5c\n"},{"id":"283028","messageId":"20160409174353.GC11873@sigill.intra.peff.net","threadId":"41955","inReplyTo":"20160409173904.GA5127@sigill.intra.peff.net","subject":"[PATCH 3/3] git_config_set_multivar_in_file: handle \"unset\" errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T17:43:54Z","receivedAt":"2016-04-09T17:43:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass off to the \"_gently\" form to do the real work, and\njust die() if it returned an error. However, our die message\nde-references \"value\", which may be NULL if the request was\nto unset a variable. Nobody using glibc noticed, because it\nsimply prints \"(null)\", which is good enough for the test\nsuite (and presumably very few people run across this in\npractice). But other libc implementations (like Solaris) may\nsegfault.\n\nLet's not only fix that, but let's make the message more\nclear about what is going on in the \"unset\" case.\n\nReported-by: \"Tom G. Christensen\" \nSigned-off-by: Jeff King \n---\n config.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d446315..3fe40c3 100644\n--- a/config.c\n+++ b/config.c\n@@ -2221,9 +2221,13 @@ void git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t const char *key, const char *value,\n \t\t\t\t const char *value_regex, int multi_replace)\n {\n-\tif (git_config_set_multivar_in_file_gently(config_filename, key, value,\n-\t\t\t\t\t\t value_regex, multi_replace))\n+\tif (!git_config_set_multivar_in_file_gently(config_filename, key, value,\n+\t\t\t\t\t\t value_regex, multi_replace))\n+\t\treturn;\n+\tif (value)\n \t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n+\telse\n+\t\tdie(_(\"could not unset '%s'\"), key);\n }\n \n int git_config_set_multivar_gently(const char *key, const char *value,\n-- \n2.8.1.245.g18e0f5c\n"},{"id":"283047","messageId":"57096374.6030608@jupiterrise.com","threadId":"41955","inReplyTo":"20160409173904.GA5127@sigill.intra.peff.net","subject":"Re: git segfaults on older Solaris releases","fromName":"Tom G. Christensen","fromEmail":"tgc@jupiterrise.com","sentAt":"2016-04-09T20:17:56Z","receivedAt":"2016-04-09T20:17:56Z","isPatch":false,"sender":{"key":"tgc@jupiterrise.com","avatar":"https://avatars.githubusercontent.com/u/912180?v=4"},"body":"On 09/04/16 19:39, Jeff King wrote:\n\n> [1/3]: config: lower-case first word of error strings\n> [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors\n> [3/3]: git_config_set_multivar_in_file: handle \"unset\" errors\n>\n\nI applied them to 2.8.1 and ran the testsuite again on Solaris 8/x86 and \nthe segfault is gone.\n\n-tgc\n"},{"id":"283050","messageId":"20160409203530.GA18989@sigill.intra.peff.net","threadId":"41955","inReplyTo":"57096374.6030608@jupiterrise.com","subject":"Re: git segfaults on older Solaris releases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-09T20:35:31Z","receivedAt":"2016-04-09T20:35:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 09, 2016 at 10:17:56PM +0200, Tom G. Christensen wrote:\n\n> On 09/04/16 19:39, Jeff King wrote:\n> \n> > [1/3]: config: lower-case first word of error strings\n> > [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors\n> > [3/3]: git_config_set_multivar_in_file: handle \"unset\" errors\n> >\n> \n> I applied them to 2.8.1 and ran the testsuite again on Solaris 8/x86 and the\n> segfault is gone.\n\nThanks for testing. By the way, I ran the whole test suite with \"--tee -v\"\nand grepped for \"(null)\", which does find this case on glibc systems. I\ndidn't see any other interesting cases (there _are_ mentions of\n\"(null)\", but they are from our code, not glibc converting NULLs). Which\nI guess is just corroborating your testing, since you would have seen\nany bad cases as segfaults.\n\n-Peff\n"},{"id":"283177","messageId":"20160412102124.GA1914@pks-pc.localdomain","threadId":"41955","inReplyTo":"20160409173904.GA5127@sigill.intra.peff.net","subject":"Re: git segfaults on older Solaris releases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-04-12T10:21:24Z","receivedAt":"2016-04-12T10:21:24Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Apr 09, 2016 at 01:39:05PM -0400, Jeff King wrote:\n> On Sat, Apr 09, 2016 at 09:02:38AM +0200, Tom G. Christensen wrote:\n> \n> > I've finished testing 2.8.1 and I found one more case where a null is being\n> > printed and causing a segfault. This happens even on Solaris 8 and 9.\n> > The failling test is t3200.63.\n> \n> Oh good, this one wasn't me. :)\n> \n> It's just a normal \"oops, we feed NULL and nobody on glibc noticed\n> because it silently replaced it with \"(null)\" case. I did find a few\n> other oddities while fixing it, though. +cc Patrick, who worked in this\n> area most recently.\n> \n> [1/3]: config: lower-case first word of error strings\n> [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors\n> [3/3]: git_config_set_multivar_in_file: handle \"unset\" errors\n> \n> I think we may want some additional improvements. While doing 1/3, I\n> noticed that many of these error messages could stand to be marked for\n> translation. As other people are already looking at mass-conversion,\n> I stopped short of doing it here (and merely contented myself with\n> throwing a conflict into their patches ;) ).\n> \n> The other thing is that 2/3 notices the error return from the\n> config-setting functions is weird. It's sometimes negative and sometimes\n> positive. I fixed this caller, but I think it's possible for the\n> negative values to leak into our exit codes:\n> \n> $ touch .git/config\n> $ git config foo.bar baz\n> error: could not lock config file .git/config: File exists\n> $ echo $?\n> 255\n> \n> I seem to recall some systems having trouble with negative error codes,\n> so we may want to make that more consistent.\n> \n> -Peff\n\nOh, yeah. Those patches look good to me, thanks for cleaning up\nmy error.\n\nPatrick\n"}]}