git segfaults on older Solaris releases

git segfaults on older Solaris releases

[Tom G. Christensen]

Hello,

While working on an update to the git packages in tgcware(1) I ran into 
segfaults when running the testsuite.

Here's what it looks like on Solaris 7/SPARC:

Core was generated by 
`/export/home/tgc/buildpkg/git/src/git-upstream/git update-index 
should-be-empty'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0xfee81ef4 in _doprnt () from /usr/lib/libc.so.1
(gdb) bt
#0  0xfee81ef4 in _doprnt () from /usr/lib/libc.so.1
#1  0xfee83ce4 in vsnprintf () from /usr/lib/libc.so.1
#2  0x00138dbc in strbuf_vaddf (sb=0xffbedd24, fmt=0x1af7b8 "%.*s%s", 
ap=0xffbedde0) at strbuf.c:279
#3  0x00139f78 in xstrvfmt (fmt=0x1af7b8 "%.*s%s", ap=0xffbedde0) at 
strbuf.c:698
#4  0x00139fb4 in xstrfmt (fmt=0x1af7b8 "%.*s%s") at strbuf.c:708
#5  0x0012a0ec in prefix_path_gently (prefix=0x0, len=0, 
remaining_prefix=0x0, path=<optimized out>) at setup.c:103
#6  0x0012a2f0 in prefix_path (prefix=0x0, len=0, path=0xffbee7fc 
"should-be-empty") at setup.c:116
#7  0x00098464 in cmd_update_index (argc=2, argv=<optimized out>, 
prefix=0x0) at builtin/update-index.c:1042
#8  0x00025900 in run_builtin (argv=0xffbee630, argc=2, p=0x1c9adc 
<commands+1260>) at git.c:346
#9  handle_builtin (argc=2, argv=0xffbee630) at git.c:536
#10 0x00025bec in run_argv (argv=0xffbee5c4, argcp=0xffbee60c) at git.c:582
#11 main (argc=2, av=<optimized out>) at git.c:690
(gdb)


The reason for the crash is simple, a null value was passed to the 's' 
format for the *printf family of functions.
To verify I modified git.c:run_builtin() so it would assign "" to prefix 
if NULL just before the status = p->fn(..) call.
This allowed t0000-basic.sh to pass where before it would fail because 
git segfaulted in multiple tests.

Passing a null value to the 's' format is explicitly documented as 
giving undefined results on Solaris, even on Solaris 11(2).
It happens that Solaris 8 and later will tolerate this without crashing, 
though I suspect at least for Solaris 8 and 9 it might require a certain 
patchlevel to do so. Earlier releases will just segfault as shown above.

I bisected it on Solaris 2.6 and found that 75faa45 was the commit that 
caused this problem to appear. The 2.6.x releases build and run fine.

I know of course that Solaris < 8 is not terribly interesting as a 
portability target so I understand if you're unwilling to fix this as it 
seems it might be a somewhat invasive change.

-tgc

1) http://jupiterrise.com/tgcware/tgcware.solaris.html
2) http://docs.oracle.com/cd/E23824_01/html/821-1465/printf-3c.html

Re: git segfaults on older Solaris releases

[Junio C Hamano]

"Tom G. Christensen" <tgc@jupiterrise.com> writes:

> The reason for the crash is simple, a null value was passed to the 's'
> format for the *printf family of functions.
> ...
> Passing a null value to the 's' format is explicitly documented as
> giving undefined results on Solaris, even on Solaris 11(2).

Do you mean

	*printf("...%.*s...", ..., 0, NULL, ...)

i.e. you saw a NULL passed only when we use %.*s with width=0?

Re: git segfaults on older Solaris releases

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Tom G. Christensen" <tgc@jupiterrise.com> writes:
>
>> The reason for the crash is simple, a null value was passed to the 's'
>> format for the *printf family of functions.
>> ...
>> Passing a null value to the 's' format is explicitly documented as
>> giving undefined results on Solaris, even on Solaris 11(2).
>
> Do you mean
>
> 	*printf("...%.*s...", ..., 0, NULL, ...)
>
> i.e. you saw a NULL passed only when we use %.*s with width=0?

So, I've looked at places where we use "%.*s" with "prefix" nearby,
and it seems that this is the only place.

The "prefix" being a NULL is a perfectly valid state throughout the
system and means a different thing than it being an empty string, so
it is valid for callers of prefix_path() and prefix_path_gently() to
pass prefix=NULL as long as they pass len=0.

So perhaps this is all we need to fix your box.

 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 3439ec6..b6c8aab 100644
--- a/setup.c
+++ b/setup.c
@@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,
 			return NULL;
 		}
 	} else {
-		sanitized = xstrfmt("%.*s%s", len, prefix, path);
+		sanitized = xstrfmt("%.*s%s", len, prefix ? prefix : "", path);
 		if (remaining_prefix)
 			*remaining_prefix = len;
 		if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {

Re: git segfaults on older Solaris releases

[David Turner]

On Thu, 2016-04-07 at 11:50 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "Tom G. Christensen" <tgc@jupiterrise.com> writes:
> > 
> > > The reason for the crash is simple, a null value was passed to
> > > the 's'
> > > format for the *printf family of functions.
> > > ...
> > > Passing a null value to the 's' format is explicitly documented
> > > as
> > > giving undefined results on Solaris, even on Solaris 11(2).
> > 
> > Do you mean
> > 
> > 	*printf("...%.*s...", ..., 0, NULL, ...)
> > 
> > i.e. you saw a NULL passed only when we use %.*s with width=0?
> 
> So, I've looked at places where we use "%.*s" with "prefix" nearby,
> and it seems that this is the only place.
> 
> The "prefix" being a NULL is a perfectly valid state throughout the
> system and means a different thing than it being an empty string, so
> it is valid for callers of prefix_path() and prefix_path_gently() to
> pass prefix=NULL as long as they pass len=0.

TIOLI: The code below does not reflect the "as long as they pass len=0"
condition.  Maybe add an assert for that?  

Re: git segfaults on older Solaris releases

[Tom G. Christensen]

On 07/04/16 20:32, Junio C Hamano wrote:
> "Tom G. Christensen" <tgc@jupiterrise.com> writes:
>
>> The reason for the crash is simple, a null value was passed to the 's'
>> format for the *printf family of functions.
>> ...
>> Passing a null value to the 's' format is explicitly documented as
>> giving undefined results on Solaris, even on Solaris 11(2).
>
> Do you mean
>
> 	*printf("...%.*s...", ..., 0, NULL, ...)
>
> i.e. you saw a NULL passed only when we use %.*s with width=0?
>

Maybe? Not sure what you're asking exactly.

I'm seing what is in the backtrace from gdb and that is prefix is NULL 
(0x0) which ends up being printed using some variant of '%s' after going 
through the various wrappers.

I hacked around it in run_builtin() as a proof and have also made some 
experiments with working around it in setup_git_directory_gently() which 
got me a bit further but it looks like there are places that do 
if(prefix) which now does not behave as expected because prefix is not NULL.

-tgc

Re: git segfaults on older Solaris releases

[Jeff King]

On Thu, Apr 07, 2016 at 11:50:46AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "Tom G. Christensen" <tgc@jupiterrise.com> writes:
> >
> >> The reason for the crash is simple, a null value was passed to the 's'
> >> format for the *printf family of functions.
> >> ...
> >> Passing a null value to the 's' format is explicitly documented as
> >> giving undefined results on Solaris, even on Solaris 11(2).

Thanks, TIL (though it is not really surprising, I guess, since some
memcpy implementations have the same problem).

> So, I've looked at places where we use "%.*s" with "prefix" nearby,
> and it seems that this is the only place.

Thank you for digging; I obviously didn't think about this issue at all
when doing the mass conversions recently.

> The "prefix" being a NULL is a perfectly valid state throughout the
> system and means a different thing than it being an empty string, so
> it is valid for callers of prefix_path() and prefix_path_gently() to
> pass prefix=NULL as long as they pass len=0.
> 
> So perhaps this is all we need to fix your box.
> 
>  setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/setup.c b/setup.c
> index 3439ec6..b6c8aab 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,
>  			return NULL;
>  		}
>  	} else {
> -		sanitized = xstrfmt("%.*s%s", len, prefix, path);
> +		sanitized = xstrfmt("%.*s%s", len, prefix ? prefix : "", path);
>  		if (remaining_prefix)
>  			*remaining_prefix = len;
>  		if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {

The original pre-75faa45ae0230b321bf72027b2274315d7e14e34 version
checked "if (len)", but I think this should be equally right.

-Peff

Re: git segfaults on older Solaris releases

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> So perhaps this is all we need to fix your box.
>> 
>>  setup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/setup.c b/setup.c
>> index 3439ec6..b6c8aab 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,
>>  			return NULL;
>>  		}
>>  	} else {
>> -		sanitized = xstrfmt("%.*s%s", len, prefix, path);
>> +		sanitized = xstrfmt("%.*s%s", len, prefix ? prefix : "", path);
>>  		if (remaining_prefix)
>>  			*remaining_prefix = len;
>>  		if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {
>
> The original pre-75faa45ae0230b321bf72027b2274315d7e14e34 version
> checked "if (len)", but I think this should be equally right.

That's a good point.  The original had

-               sanitized = xmalloc(len + strlen(path) + 1);
-               if (len)
-                       memcpy(sanitized, prefix, len);
-               strcpy(sanitized + len, path);
+               sanitized = xstrfmt("%.*s%s", len, prefix, path);

and for a brief moment I was wondering why the strlen() did not
barf, until I realized that was on path not prefix.

-- >8 --
Subject: setup.c: do not feed NULL to "%.*s" even with the precision 0

A recent update 75faa45a (replace trivial malloc + sprintf / strcpy
calls with xstrfmt, 2015-09-24) rewrote

	prepare an empty buffer
	if (len)
        	append the first len bytes of "prefix" to the buffer
	append "path" to the buffer

that computed "path", optionally prefixed by "prefix", into

	xstrfmt("%.*s%s", len, prefix, path);

However, passing a NULL pointer to the printf(3) family of functions
to format it with %s conversion, even with the precision 0, i.e.

	xstrfmt("%.*s", 0, NULL)

yields undefined results, at least on some platforms.  

Avoid this problem by substituting prefix with "" when len==0, as
prefix can legally be NULL in that case.  This would mimick the
intent of the original code better.

Reported-by: "Tom G. Christensen" <tgc@jupiterrise.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 3439ec6..b4a92fe 100644
--- a/setup.c
+++ b/setup.c
@@ -103,7 +103,7 @@ char *prefix_path_gently(const char *prefix, int len,
 			return NULL;
 		}
 	} else {
-		sanitized = xstrfmt("%.*s%s", len, prefix, path);
+		sanitized = xstrfmt("%.*s%s", len, len ? prefix : "", path);
 		if (remaining_prefix)
 			*remaining_prefix = len;
 		if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {

Re: git segfaults on older Solaris releases

[Tom G. Christensen]

On 07/04/16 20:50, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> So perhaps this is all we need to fix your box.
>
>   setup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>

Yes that seems to work very well.

I applied this patch to 2.8.0 and have completed a testsuite run on 
Solaris 2.6/x86 with only a few unrelated problems.
I will continue testing on the other Solaris < 10 releases but I do not 
expect any difference in the outcome.

-tgc

Re: git segfaults on older Solaris releases

[Jeff King]

On Thu, Apr 07, 2016 at 12:37:53PM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: setup.c: do not feed NULL to "%.*s" even with the precision 0
> 
> A recent update 75faa45a (replace trivial malloc + sprintf / strcpy
> calls with xstrfmt, 2015-09-24) rewrote
> 
> 	prepare an empty buffer
> 	if (len)
>         	append the first len bytes of "prefix" to the buffer
> 	append "path" to the buffer
> 
> that computed "path", optionally prefixed by "prefix", into
> 
> 	xstrfmt("%.*s%s", len, prefix, path);
> 
> However, passing a NULL pointer to the printf(3) family of functions
> to format it with %s conversion, even with the precision 0, i.e.
> 
> 	xstrfmt("%.*s", 0, NULL)
> 
> yields undefined results, at least on some platforms.  
> 
> Avoid this problem by substituting prefix with "" when len==0, as
> prefix can legally be NULL in that case.  This would mimick the
> intent of the original code better.
> 
> Reported-by: "Tom G. Christensen" <tgc@jupiterrise.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Nicely explained.

Acked-by: Jeff King <peff@peff.net>

Thanks.

-Peff

Re: git segfaults on older Solaris releases

[Tom G. Christensen]

On 07/04/16 22:19, Tom G. Christensen wrote:
> On 07/04/16 20:50, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> So perhaps this is all we need to fix your box.
>>
>>   setup.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> I applied this patch to 2.8.0 and have completed a testsuite run on
> Solaris 2.6/x86 with only a few unrelated problems.
> I will continue testing on the other Solaris < 10 releases but I do not
> expect any difference in the outcome.
>

I've finished testing 2.8.1 and I found one more case where a null is 
being printed and causing a segfault. This happens even on Solaris 8 and 9.
The failling test is t3200.63.

Here is the backtrace from a Solaris 8/SPARC machine:

(gdb) core trash directory.t3200-branch/core
[New LWP 1]
[Thread debugging using libthread_db enabled]
[New Thread 1 (LWP 1)]
Core was generated by `/export/home/tgc/buildpkg/git/src/git-2.8.1/git 
branch --unset-upstream'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0xfecb32cc in strlen () from /usr/lib/libc.so.1
(gdb) bt
#0  0xfecb32cc in strlen () from /usr/lib/libc.so.1
#1  0xfed06508 in _doprnt () from /usr/lib/libc.so.1
#2  0xfed08690 in vfprintf () from /usr/lib/libc.so.1
#3  0x001487bc in vreportf (prefix=<optimized out>, err=<optimized out>, 
params=0xffbfe408) at usage.c:23
#4  0x0014881c in die_builtin (err=0x198f90 "Could not set '%s' to 
'%s'", params=0xffbfe408) at usage.c:35
#5  0x00148934 in die (err=0x198f90 "Could not set '%s' to '%s'") at 
usage.c:108
#6  0x000af1b0 in git_config_set_multivar_in_file (value=0x0, 
key=0x1ecca0 "branch.master.remote",
     config_filename=<optimized out>, value_regex=<optimized out>, 
multi_replace=<optimized out>) at config.c:2226
#7  git_config_set_multivar_in_file (config_filename=0x0, key=0x1ecca0 
"branch.master.remote", value=0x0,
     value_regex=0x0, multi_replace=1) at config.c:2220
#8  0x0003aa6c in cmd_branch (argc=0, argv=0xffbfec00, prefix=<optimized 
out>) at builtin/branch.c:793
#9  0x000255e8 in run_builtin (argv=0xffbfec00, argc=2, p=0x1c365c 
<commands+84>) at git.c:353
#10 handle_builtin (argc=2, argv=0xffbfec00) at git.c:540
#11 0x00168ecc in run_argv (argv=0xffbfeb30, argcp=0xffbfebdc) at git.c:594
#12 main (argc=2, av=<optimized out>) at git.c:701
(gdb)


-tgc

Re: git segfaults on older Solaris releases

[Jeff King]

On Sat, Apr 09, 2016 at 09:02:38AM +0200, Tom G. Christensen wrote:

> I've finished testing 2.8.1 and I found one more case where a null is being
> printed and causing a segfault. This happens even on Solaris 8 and 9.
> The failling test is t3200.63.

Oh good, this one wasn't me. :)

It's just a normal "oops, we feed NULL and nobody on glibc noticed
because it silently replaced it with "(null)" case. I did find a few
other oddities while fixing it, though. +cc Patrick, who worked in this
area most recently.

  [1/3]: config: lower-case first word of error strings
  [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors
  [3/3]: git_config_set_multivar_in_file: handle "unset" errors

I think we may want some additional improvements. While doing 1/3, I
noticed that many of these error messages could stand to be marked for
translation. As other people are already looking at mass-conversion,
I stopped short of doing it here (and merely contented myself with
throwing a conflict into their patches ;) ).

The other thing is that 2/3 notices the error return from the
config-setting functions is weird. It's sometimes negative and sometimes
positive. I fixed this caller, but I think it's possible for the
negative values to leak into our exit codes:

  $ touch .git/config
  $ git config foo.bar baz
  error: could not lock config file .git/config: File exists
  $ echo $?
  255

I seem to recall some systems having trouble with negative error codes,
so we may want to make that more consistent.

-Peff

[PATCH 1/3] config: lower-case first word of error strings

[Jeff King]

This follows our usual style (both throughout git, and
throughout the rest of this file).

This covers the whole file, but note that I left the capitalization in
the multi-sentence:

  error: malformed value...
  error: Must be one of ...

because it helps make it clear that we are starting a new sentence in
the second one.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index 8f66519..6b81931 100644
--- a/config.c
+++ b/config.c
@@ -108,7 +108,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 
 	expanded = expand_user_path(path);
 	if (!expanded)
-		return error("Could not expand include path '%s'", path);
+		return error("could not expand include path '%s'", path);
 	path = expanded;
 
 	/*
@@ -950,7 +950,7 @@ static int git_default_branch_config(const char *var, const char *value)
 		else if (!strcmp(value, "always"))
 			autorebase = AUTOREBASE_ALWAYS;
 		else
-			return error("Malformed value for %s", var);
+			return error("malformed value for %s", var);
 		return 0;
 	}
 
@@ -976,7 +976,7 @@ static int git_default_push_config(const char *var, const char *value)
 		else if (!strcmp(value, "current"))
 			push_default = PUSH_DEFAULT_CURRENT;
 		else {
-			error("Malformed value for %s: %s", var, value);
+			error("malformed value for %s: %s", var, value);
 			return error("Must be one of nothing, matching, simple, "
 				     "upstream or current.");
 		}
@@ -2223,7 +2223,7 @@ void git_config_set_multivar_in_file(const char *config_filename,
 {
 	if (git_config_set_multivar_in_file_gently(config_filename, key, value,
 						   value_regex, multi_replace) < 0)
-		die(_("Could not set '%s' to '%s'"), key, value);
+		die(_("could not set '%s' to '%s'"), key, value);
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
@@ -2404,7 +2404,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 #undef config_error_nonbool
 int config_error_nonbool(const char *var)
 {
-	return error("Missing value for '%s'", var);
+	return error("missing value for '%s'", var);
 }
 
 int parse_config_key(const char *var,
-- 
2.8.1.245.g18e0f5c

[PATCH 2/3] git_config_set_multivar_in_file: all non-zero returns are errors

[Jeff King]

This function is just a thin wrapper for the "_gently" form
of the function. But the gently form is designed to feed
builtin/config.c, which passes our return code directly to
its exit status, and thus uses positive error values for
some cases. We check only negative values, meaning we would
fail to die in some cases (e.g., a malformed key).

This may or may not be triggerable in practice; we tend to
use this non-gentle form only when setting internal
variables, which would not have malformed keys.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 6b81931..d446315 100644
--- a/config.c
+++ b/config.c
@@ -2222,7 +2222,7 @@ void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *value_regex, int multi_replace)
 {
 	if (git_config_set_multivar_in_file_gently(config_filename, key, value,
-						   value_regex, multi_replace) < 0)
+						   value_regex, multi_replace))
 		die(_("could not set '%s' to '%s'"), key, value);
 }
 
-- 
2.8.1.245.g18e0f5c

[PATCH 3/3] git_config_set_multivar_in_file: handle "unset" errors

[Jeff King]

We pass off to the "_gently" form to do the real work, and
just die() if it returned an error. However, our die message
de-references "value", which may be NULL if the request was
to unset a variable. Nobody using glibc noticed, because it
simply prints "(null)", which is good enough for the test
suite (and presumably very few people run across this in
practice). But other libc implementations (like Solaris) may
segfault.

Let's not only fix that, but let's make the message more
clear about what is going on in the "unset" case.

Reported-by: "Tom G. Christensen" <tgc@jupiterrise.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index d446315..3fe40c3 100644
--- a/config.c
+++ b/config.c
@@ -2221,9 +2221,13 @@ void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key, const char *value,
 				     const char *value_regex, int multi_replace)
 {
-	if (git_config_set_multivar_in_file_gently(config_filename, key, value,
-						   value_regex, multi_replace))
+	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
+						    value_regex, multi_replace))
+		return;
+	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
+	else
+		die(_("could not unset '%s'"), key);
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
-- 
2.8.1.245.g18e0f5c

Re: git segfaults on older Solaris releases

[Tom G. Christensen]

On 09/04/16 19:39, Jeff King wrote:

>    [1/3]: config: lower-case first word of error strings
>    [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors
>    [3/3]: git_config_set_multivar_in_file: handle "unset" errors
>

I applied them to 2.8.1 and ran the testsuite again on Solaris 8/x86 and 
the segfault is gone.

-tgc

Re: git segfaults on older Solaris releases

[Jeff King]

On Sat, Apr 09, 2016 at 10:17:56PM +0200, Tom G. Christensen wrote:

> On 09/04/16 19:39, Jeff King wrote:
> 
> >   [1/3]: config: lower-case first word of error strings
> >   [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors
> >   [3/3]: git_config_set_multivar_in_file: handle "unset" errors
> >
> 
> I applied them to 2.8.1 and ran the testsuite again on Solaris 8/x86 and the
> segfault is gone.

Thanks for testing. By the way, I ran the whole test suite with "--tee -v"
and grepped for "(null)", which does find this case on glibc systems. I
didn't see any other interesting cases (there _are_ mentions of
"(null)", but they are from our code, not glibc converting NULLs). Which
I guess is just corroborating your testing, since you would have seen
any bad cases as segfaults.

-Peff

Re: git segfaults on older Solaris releases

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On Sat, Apr 09, 2016 at 01:39:05PM -0400, Jeff King wrote:
> On Sat, Apr 09, 2016 at 09:02:38AM +0200, Tom G. Christensen wrote:
> 
> > I've finished testing 2.8.1 and I found one more case where a null is being
> > printed and causing a segfault. This happens even on Solaris 8 and 9.
> > The failling test is t3200.63.
> 
> Oh good, this one wasn't me. :)
> 
> It's just a normal "oops, we feed NULL and nobody on glibc noticed
> because it silently replaced it with "(null)" case. I did find a few
> other oddities while fixing it, though. +cc Patrick, who worked in this
> area most recently.
> 
>   [1/3]: config: lower-case first word of error strings
>   [2/3]: git_config_set_multivar_in_file: all non-zero returns are errors
>   [3/3]: git_config_set_multivar_in_file: handle "unset" errors
> 
> I think we may want some additional improvements. While doing 1/3, I
> noticed that many of these error messages could stand to be marked for
> translation. As other people are already looking at mass-conversion,
> I stopped short of doing it here (and merely contented myself with
> throwing a conflict into their patches ;) ).
> 
> The other thing is that 2/3 notices the error return from the
> config-setting functions is weird. It's sometimes negative and sometimes
> positive. I fixed this caller, but I think it's possible for the
> negative values to leak into our exit codes:
> 
>   $ touch .git/config
>   $ git config foo.bar baz
>   error: could not lock config file .git/config: File exists
>   $ echo $?
>   255
> 
> I seem to recall some systems having trouble with negative error codes,
> so we may want to make that more consistent.
> 
> -Peff

Oh, yeah. Those patches look good to me, thanks for cleaning up
my error.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]