git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Tom G. Christensen" <tgc@jupiterrise.com>, git@vger.kernel.org
Subject: Re: git segfaults on older Solaris releases
Date: Thu, 07 Apr 2016 12:37:53 -0700	[thread overview]
Message-ID: <xmqq8u0pyzu6.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160407190709.GC4478@sigill.intra.peff.net> (Jeff King's message of "Thu, 7 Apr 2016 15:07:10 -0400")

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)) {

  reply	other threads:[~2016-04-07 19:38 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-07 18:18 git segfaults on older Solaris releases Tom G. Christensen
2016-04-07 18:32 ` Junio C Hamano
2016-04-07 18:50   ` Junio C Hamano
2016-04-07 18:56     ` David Turner
2016-04-07 19:07     ` Jeff King
2016-04-07 19:37       ` Junio C Hamano [this message]
2016-04-07 20:24         ` Jeff King
2016-04-07 20:19     ` Tom G. Christensen
2016-04-09  7:02       ` Tom G. Christensen
2016-04-09 17:39         ` Jeff King
2016-04-09 17:42           ` [PATCH 1/3] config: lower-case first word of error strings Jeff King
2016-04-09 17:42           ` [PATCH 2/3] git_config_set_multivar_in_file: all non-zero returns are errors Jeff King
2016-04-09 17:43           ` [PATCH 3/3] git_config_set_multivar_in_file: handle "unset" errors Jeff King
2016-04-09 20:17           ` git segfaults on older Solaris releases Tom G. Christensen
2016-04-09 20:35             ` Jeff King
2016-04-12 10:21           ` Patrick Steinhardt
2016-04-07 18:58   ` Tom G. Christensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8u0pyzu6.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=tgc@jupiterrise.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).