Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Tue, Sep 15, 2015 at 01:38:42PM -0700, Stefan Beller wrote:

> Some off topic thoughts:
> 
> Having an "assert" behavior is not a good user experience though
> and should be fixed. To fix it we need stack traces. And the git
> version. And telling the user to send it to the mailing list.

Yes, any assert (or die("BUG")) should generally result in a code change
to make it never happen again.

I have been tempted to replace our die("BUG") calls with a BUG() macro,
so that we can do more fancy things (even if it is just adding
__FILE__:__LINE__ and a message to contact the list).

> I wonder if we want to include a trace where possible (i.e.
> when compiled with gcc or other precompiler conditions)
> into these assertive behaviors.

That would be nice. I took a look at this back in April:

  http://thread.gmane.org/gmane.comp.version-control.git/267643/focus=267755

but I think I convinced myself that we are better off relying on people
running gdb. If we do anything, it should be to make doing so easier.

> I'd guess we have an assertive behavior if die("BUG:...") is called,
> so maybe we can just check inside of die if we want to print
> a stack trace additionally ?

I guess we could look for starts_with(fmt, "BUG: ") in die(). I think it
would be a bit less gross to convert those calls to a BUG() macro (we
can't get accurate __LINE__ information without it, though in practice
the die messages are usually unique enough to be helpful).

I think a core file is even more useful than a backtrace. Having BUG()
call abort() would be even more useful, I think.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:

> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
> > -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
> > +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
> > ce->ce_mode);
> 
> Interesting. I wonder if there are any (old/broken) compilers which
> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?

Good point. I've changed it to sizeof(ownbuf[0]).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Junio C Hamano]

Jeff King  writes:

> I think a core file is even more useful than a backtrace. Having BUG()
> call abort() would be even more useful, I think.

Sounds like a plan ;-)
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Stefan Beller]

On Wed, Sep 16, 2015 at 12:07 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Wed, Sep 16, 2015 at 11:24:27AM -0700, Junio C Hamano wrote:
>>
>>> Jeff King  writes:
>>>
>>> > On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:
>>> >
>>> >> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
>>> >> > -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
>>> >> > +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
>>> >> > ce->ce_mode);
>>> >>
>>> >> Interesting. I wonder if there are any (old/broken) compilers which
>>> >> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?
>>> >
>>> > Good point. I've changed it to sizeof(ownbuf[0]).
>>>
>>> Panda brain is lost here.  What's the difference, other than that we
>>> will now appear to be measuring the size of the thing at index 0
>>> while using that size to stuff data into a different location?  All
>>> elements of the array are of the same size so there wouldn't be any
>>> difference either way, no?
>>
>> Correct. The original is sane and gcc does the right thing. The question
>> is whether some compiler would complain that "stage" is not a constant
>> in the sizeof() expression. I don't know if any compiler would do so,
>> but it is easy enough to be conservative.
>
> Wouldn't such a compiler also complain if you did this, though?
>
> int *pointer_to_int;
> size_t sz = sizeof(*pointer_to_int);
>
> You (as a complier) do not know exactly where ownbuf[stage] is,
> because "stage" is unknown to you.  In the same way, you do not know
> exactly where *pointer_to_int is.  But of course, what the sizeof()
> operator is being asked is the size of the thing, which does not
> depend on where the thing is.  If you (as a compiler) does not know
> that and complain to ownbuf[stage], wouldn't you complain to the
> pointer dereference, too?
>
> A more important reason I am reluctant to see this:
>
> xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
>
> is that it looks strange in the same way as this
>
> memcpy(ownbuf[stage], src, sizeof(ownbuf[0]));
>
> looks strange.  "We use the size of one thing to stuff into another".
>
> That will make future readers wonder "Is this a typo, and if it is,
> which index is a mistake I can fix?" and may lead to an unnecessary
> confusion.  I do not want to see a correctly written
>
> xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
>
> turned into
>
> xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);
>
> out of such a confusion.

So we could just not use the bracket notation, but the pointer then?

xsnprintf(ownbuf[stage], sizeof(*ownbuf), "%o", ...);

IMHO that would reasonably well tell you that we just care about the
size of one element there.

A funny thought:

 xsnprintf(ownbuf[stage], sizeof(ownbuf[-1]), "%o", ...);

should work as well as any reader would question the sanity
of a negative index.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Junio C Hamano]

Jeff King  writes:

> On Wed, Sep 16, 2015 at 11:24:27AM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:
>> >
>> >> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
>> >> > -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
>> >> > +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
>> >> > ce->ce_mode);
>> >> 
>> >> Interesting. I wonder if there are any (old/broken) compilers which
>> >> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?
>> >
>> > Good point. I've changed it to sizeof(ownbuf[0]).
>> 
>> Panda brain is lost here.  What's the difference, other than that we
>> will now appear to be measuring the size of the thing at index 0
>> while using that size to stuff data into a different location?  All
>> elements of the array are of the same size so there wouldn't be any
>> difference either way, no?
>
> Correct. The original is sane and gcc does the right thing. The question
> is whether some compiler would complain that "stage" is not a constant
> in the sizeof() expression. I don't know if any compiler would do so,
> but it is easy enough to be conservative.

Wouldn't such a compiler also complain if you did this, though?

int *pointer_to_int;
size_t sz = sizeof(*pointer_to_int);

You (as a complier) do not know exactly where ownbuf[stage] is,
because "stage" is unknown to you.  In the same way, you do not know
exactly where *pointer_to_int is.  But of course, what the sizeof()
operator is being asked is the size of the thing, which does not
depend on where the thing is.  If you (as a compiler) does not know
that and complain to ownbuf[stage], wouldn't you complain to the
pointer dereference, too?

A more important reason I am reluctant to see this:

xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);

is that it looks strange in the same way as this

memcpy(ownbuf[stage], src, sizeof(ownbuf[0]));

looks strange.  "We use the size of one thing to stuff into another".

That will make future readers wonder "Is this a typo, and if it is,
which index is a mistake I can fix?" and may lead to an unnecessary
confusion.  I do not want to see a correctly written

xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);

turned into

xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);

out of such a confusion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Junio C Hamano]

Jeff King  writes:

> On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:
>
>> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
>> > -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
>> > +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
>> > ce->ce_mode);
>> 
>> Interesting. I wonder if there are any (old/broken) compilers which
>> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?
>
> Good point. I've changed it to sizeof(ownbuf[0]).

Panda brain is lost here.  What's the difference, other than that we
will now appear to be measuring the size of the thing at index 0
while using that size to stuff data into a different location?  All
elements of the array are of the same size so there wouldn't be any
difference either way, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Wed, Sep 16, 2015 at 12:07:30PM -0700, Junio C Hamano wrote:

> > Correct. The original is sane and gcc does the right thing. The question
> > is whether some compiler would complain that "stage" is not a constant
> > in the sizeof() expression. I don't know if any compiler would do so,
> > but it is easy enough to be conservative.
> 
> Wouldn't such a compiler also complain if you did this, though?
> 
>   int *pointer_to_int;
> size_t sz = sizeof(*pointer_to_int);
>
> You (as a complier) do not know exactly where ownbuf[stage] is,
> because "stage" is unknown to you.  In the same way, you do not know
> exactly where *pointer_to_int is.  But of course, what the sizeof()
> operator is being asked is the size of the thing, which does not
> depend on where the thing is.  If you (as a compiler) does not know
> that and complain to ownbuf[stage], wouldn't you complain to the
> pointer dereference, too?

I think it depends on how crappily the compiler is implemented. I agree
that sizeof(ownbuf[stage]) is a reasonable thing to ask for without
knowing the exact value of stage. But I could also see it being a
mistake an old or badly written compiler might make.

But we are theorizing without data. I'm happy to leave it as in my
original and wait to see if anybody ever complains.

> A more important reason I am reluctant to see this:
> 
>   xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
> 
> is that it looks strange in the same way as this
> 
>   memcpy(ownbuf[stage], src, sizeof(ownbuf[0]));
> 
> looks strange.  "We use the size of one thing to stuff into another".
> 
> That will make future readers wonder "Is this a typo, and if it is,
> which index is a mistake I can fix?" and may lead to an unnecessary
> confusion.  I do not want to see a correctly written
> 
>   xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
> 
> turned into
> 
>   xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);
> 
> out of such a confusion.

I think that's a reasonable concern.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Wed, Sep 16, 2015 at 12:19:10PM -0700, Stefan Beller wrote:

> > That will make future readers wonder "Is this a typo, and if it is,
> > which index is a mistake I can fix?" and may lead to an unnecessary
> > confusion.  I do not want to see a correctly written
> >
> > xsnprintf(ownbuf[stage], sizeof(ownbuf[0]), "%o", ...);
> >
> > turned into
> >
> > xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", ...);
> >
> > out of such a confusion.
> 
> So we could just not use the bracket notation, but the pointer then?
> 
> xsnprintf(ownbuf[stage], sizeof(*ownbuf), "%o", ...);

IMHO that is even more confusing, as I expect sizeof(*ownbuf) to
generally be dereferencing a pointer to a struct, and we would be
writing to "ownbuf". There's not anything _wrong_ with what you've
written, it's just using a syntax that in my mind generally applies to a
different idiom, and I'd wonder if the writer got it wrong.

> IMHO that would reasonably well tell you that we just care about the
> size of one element there.
> 
> A funny thought:
> 
>  xsnprintf(ownbuf[stage], sizeof(ownbuf[-1]), "%o", ...);
> 
> should work as well as any reader would question the sanity
> of a negative index.

I'm not sure what the standard would have to say on that, as the
expression invokes undefined behavior (but of course we're not really
using the expression, only asking for the size). I tried to find any
mention of non-constant indices with sizeof() in the standard, but
couldn't.

I think at this point I'm inclined to switch it back to the original
sizeof(ownbuf[stage]), and we can deal with this if and when any
compiler actually complains.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Wed, Sep 16, 2015 at 11:24:27AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Tue, Sep 15, 2015 at 11:19:21PM -0400, Eric Sunshine wrote:
> >
> >> > strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
> >> > -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
> >> > +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
> >> > ce->ce_mode);
> >> 
> >> Interesting. I wonder if there are any (old/broken) compilers which
> >> would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?
> >
> > Good point. I've changed it to sizeof(ownbuf[0]).
> 
> Panda brain is lost here.  What's the difference, other than that we
> will now appear to be measuring the size of the thing at index 0
> while using that size to stuff data into a different location?  All
> elements of the array are of the same size so there wouldn't be any
> difference either way, no?

Correct. The original is sane and gcc does the right thing. The question
is whether some compiler would complain that "stage" is not a constant
in the sizeof() expression. I don't know if any compiler would do so,
but it is easy enough to be conservative.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Eric Sunshine]

On Tue, Sep 15, 2015 at 11:36 AM, Jeff King  wrote:
> We sometimes sprintf into static buffers when we know that
> the size of the buffer is large enough to fit the input
> (either because it's a constant, or because it's numeric
> input that is bounded in size). Likewise with strcpy of
> constant strings.
>
> However, these sites make it hard to audit sprintf and
> strcpy calls for buffer overflows, as a reader has to
> cross-reference the size of the array with the input. Let's
> use xsnprintf instead, which communicates to a reader that
> we don't expect this to overflow (and catches the mistake in
> case we do).
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index 1a1eafa..1d66111 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -23,7 +23,7 @@ static int merge_entry(int pos, const char *path)
> break;
> found++;
> strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
> -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
> +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
> ce->ce_mode);

Interesting. I wonder if there are any (old/broken) compilers which
would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?

> arguments[stage] = hexbuf[stage];
> arguments[stage + 4] = ownbuf[stage];
> } while (++pos < active_nr);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Junio C Hamano]

Ramsay Jones  writes:

> How about using strlcpy() instead? Thus:
>
> - strcpy(header.name, "pax_global_header");
> + strlcpy(header.name, "pax_global_header", sizeof(header.name));
>
> Ditto for other similar (strcpy->xsnprintf) hunks below.

Please do not advocate use of strlcpy(), which substitutes
overwriting beyond the end of the buffer (which is a bug) with a
silent truncation (which is almost always another bug, unless in a
very narrow case of producing a non-essential string result where
loss of information does not matter).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Ramsay Jones]

On 15/09/15 16:36, Jeff King wrote:
> We sometimes sprintf into static buffers when we know that
> the size of the buffer is large enough to fit the input
> (either because it's a constant, or because it's numeric
> input that is bounded in size). Likewise with strcpy of
> constant strings.
>
> However, these sites make it hard to audit sprintf and
> strcpy calls for buffer overflows, as a reader has to
> cross-reference the size of the array with the input. Let's
> use xsnprintf instead, which communicates to a reader that
> we don't expect this to overflow (and catches the mistake in
> case we do).
>
> Signed-off-by: Jeff King 
> ---
> These are all pretty trivial; the obvious thing to get wrong is that
> "sizeof(buf)" is not the correct length if "buf" is a pointer. I
> considered a macro wrapper like:
>
>   #define xsnprintf_array(dst, fmt, ...) \
>   xsnprintf(dst, sizeof(dst) + BARF_UNLESS_AN_ARRAY(dst), \
> fmt, __VA_ARGS__)
>
> but obviously that requires variadic macro support.
>
>  archive-tar.c |  2 +-
>  builtin/gc.c  |  2 +-
>  builtin/init-db.c | 11 ++-
>  builtin/ls-tree.c |  9 +
>  builtin/merge-index.c |  2 +-
>  builtin/merge-recursive.c |  2 +-
>  builtin/read-tree.c   |  2 +-
>  builtin/unpack-file.c |  2 +-
>  compat/mingw.c|  8 +---
>  compat/winansi.c  |  2 +-
>  connect.c |  2 +-
>  convert.c |  3 ++-
>  daemon.c  |  4 ++--
>  diff.c| 12 ++--
>  http-push.c   |  2 +-
>  http.c|  6 +++---
>  ll-merge.c| 12 ++--
>  refs.c|  8 
>  sideband.c|  4 ++--
>  strbuf.c  |  4 ++--
>  20 files changed, 52 insertions(+), 47 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index b6b30bb..d543f93 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -301,7 +301,7 @@ static int write_global_extended_header(struct 
> archiver_args *args)
>   memset(, 0, sizeof(header));
>   *header.typeflag = TYPEFLAG_GLOBAL_HEADER;
>   mode = 0100666;
> - strcpy(header.name, "pax_global_header");
> + xsnprintf(header.name, sizeof(header.name), "pax_global_header");

How about using strlcpy() instead? Thus:

-   strcpy(header.name, "pax_global_header");
+   strlcpy(header.name, "pax_global_header", sizeof(header.name));

Ditto for other similar (strcpy->xsnprintf) hunks below.

ATB,
Ramsay Jones

>   prepare_header(args, , mode, ext_header.len);
>   write_blocked(, sizeof(header));
>   write_blocked(ext_header.buf, ext_header.len);
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 0ad8d30..57584bc 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -194,7 +194,7 @@ static const char *lock_repo_for_gc(int force, pid_t* 
> ret_pid)
>   return NULL;
>  
>   if (gethostname(my_host, sizeof(my_host)))
> - strcpy(my_host, "unknown");
> + xsnprintf(my_host, sizeof(my_host), "unknown");
>  
>   pidfile_path = git_pathdup("gc.pid");
>   fd = hold_lock_file_for_update(, pidfile_path,
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 69323e1..e7d0e31 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)
>   }
>  
>   /* This forces creation of new config file */
> - sprintf(repo_version_string, "%d", GIT_REPO_VERSION);
> + xsnprintf(repo_version_string, sizeof(repo_version_string),
> +   "%d", GIT_REPO_VERSION);
>   git_config_set("core.repositoryformatversion", repo_version_string);
>  
>   path[len] = 0;
> @@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int 
> flags)
>*/
>   if (shared_repository < 0)
>   /* force to the mode value */
> - sprintf(buf, "0%o", -shared_repository);
> + xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);
>   else if (shared_repository == PERM_GROUP)
> - sprintf(buf, "%d", OLD_PERM_GROUP);
> + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
>   else if (shared_repository == PERM_EVERYBODY)
> - sprintf(buf, "%d", OLD_PERM_EVERYBODY);
> + xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
>   else
> - die("oops");
> + die("BUG: invalid value for shared_repository");
>   git_config_set("core.sharedrepository", buf);
>   git_config_set("receive.denyNonFastforwards", "true");
>   }
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 3b04a0f..0e30d86 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -96,12 +96,13 @@ static int 

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

On Tue, Sep 15, 2015 at 07:32:29PM +0100, Ramsay Jones wrote:

> > diff --git a/archive-tar.c b/archive-tar.c
> > index b6b30bb..d543f93 100644
> > --- a/archive-tar.c
> > +++ b/archive-tar.c
> > @@ -301,7 +301,7 @@ static int write_global_extended_header(struct 
> > archiver_args *args)
> > memset(, 0, sizeof(header));
> > *header.typeflag = TYPEFLAG_GLOBAL_HEADER;
> > mode = 0100666;
> > -   strcpy(header.name, "pax_global_header");
> > +   xsnprintf(header.name, sizeof(header.name), "pax_global_header");
> 
> How about using strlcpy() instead? Thus:
> 
> - strcpy(header.name, "pax_global_header");
> + strlcpy(header.name, "pax_global_header", sizeof(header.name));
> 
> Ditto for other similar (strcpy->xsnprintf) hunks below.

That misses the "assert" behavior of xsnprintf. We are preventing
overflow here, but also truncation. What should happen if
"pax_global_header" does not fit in header.name? I think complaining
loudly and immediately is the most helpful thing, because it is surely a
programming error.

We could make xstrlcpy(), of course, but I don't see much point when
xsnprintf does the same thing (and more).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Ramsay Jones]

On 15/09/15 19:42, Jeff King wrote:
> On Tue, Sep 15, 2015 at 07:32:29PM +0100, Ramsay Jones wrote:
>
>>> diff --git a/archive-tar.c b/archive-tar.c
>>> index b6b30bb..d543f93 100644
>>> --- a/archive-tar.c
>>> +++ b/archive-tar.c
>>> @@ -301,7 +301,7 @@ static int write_global_extended_header(struct 
>>> archiver_args *args)
>>> memset(, 0, sizeof(header));
>>> *header.typeflag = TYPEFLAG_GLOBAL_HEADER;
>>> mode = 0100666;
>>> -   strcpy(header.name, "pax_global_header");
>>> +   xsnprintf(header.name, sizeof(header.name), "pax_global_header");
>> How about using strlcpy() instead? Thus:
>>
>> -strcpy(header.name, "pax_global_header");
>> +strlcpy(header.name, "pax_global_header", sizeof(header.name));
>>
>> Ditto for other similar (strcpy->xsnprintf) hunks below.
> That misses the "assert" behavior of xsnprintf. We are preventing
> overflow here, but also truncation. What should happen if
> "pax_global_header" does not fit in header.name? I think complaining
> loudly and immediately is the most helpful thing, because it is surely a
> programming error.
>
> We could make xstrlcpy(), of course, but I don't see much point when
> xsnprintf does the same thing (and more).

Heh, I just sent an email about patch 22/67 which says similar things. I don't 
feel
too strongly, either way, but I have a slight preference for the use of 
[x]strlcpy()
in these cases.

I have to stop at patch #22 for now.

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Jeff King]

We sometimes sprintf into static buffers when we know that
the size of the buffer is large enough to fit the input
(either because it's a constant, or because it's numeric
input that is bounded in size). Likewise with strcpy of
constant strings.

However, these sites make it hard to audit sprintf and
strcpy calls for buffer overflows, as a reader has to
cross-reference the size of the array with the input. Let's
use xsnprintf instead, which communicates to a reader that
we don't expect this to overflow (and catches the mistake in
case we do).

Signed-off-by: Jeff King 
---
These are all pretty trivial; the obvious thing to get wrong is that
"sizeof(buf)" is not the correct length if "buf" is a pointer. I
considered a macro wrapper like:

  #define xsnprintf_array(dst, fmt, ...) \
xsnprintf(dst, sizeof(dst) + BARF_UNLESS_AN_ARRAY(dst), \
  fmt, __VA_ARGS__)

but obviously that requires variadic macro support.

 archive-tar.c |  2 +-
 builtin/gc.c  |  2 +-
 builtin/init-db.c | 11 ++-
 builtin/ls-tree.c |  9 +
 builtin/merge-index.c |  2 +-
 builtin/merge-recursive.c |  2 +-
 builtin/read-tree.c   |  2 +-
 builtin/unpack-file.c |  2 +-
 compat/mingw.c|  8 +---
 compat/winansi.c  |  2 +-
 connect.c |  2 +-
 convert.c |  3 ++-
 daemon.c  |  4 ++--
 diff.c| 12 ++--
 http-push.c   |  2 +-
 http.c|  6 +++---
 ll-merge.c| 12 ++--
 refs.c|  8 
 sideband.c|  4 ++--
 strbuf.c  |  4 ++--
 20 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index b6b30bb..d543f93 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -301,7 +301,7 @@ static int write_global_extended_header(struct 
archiver_args *args)
memset(, 0, sizeof(header));
*header.typeflag = TYPEFLAG_GLOBAL_HEADER;
mode = 0100666;
-   strcpy(header.name, "pax_global_header");
+   xsnprintf(header.name, sizeof(header.name), "pax_global_header");
prepare_header(args, , mode, ext_header.len);
write_blocked(, sizeof(header));
write_blocked(ext_header.buf, ext_header.len);
diff --git a/builtin/gc.c b/builtin/gc.c
index 0ad8d30..57584bc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -194,7 +194,7 @@ static const char *lock_repo_for_gc(int force, pid_t* 
ret_pid)
return NULL;
 
if (gethostname(my_host, sizeof(my_host)))
-   strcpy(my_host, "unknown");
+   xsnprintf(my_host, sizeof(my_host), "unknown");
 
pidfile_path = git_pathdup("gc.pid");
fd = hold_lock_file_for_update(, pidfile_path,
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 69323e1..e7d0e31 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,8 @@ static int create_default_files(const char *template_path)
}
 
/* This forces creation of new config file */
-   sprintf(repo_version_string, "%d", GIT_REPO_VERSION);
+   xsnprintf(repo_version_string, sizeof(repo_version_string),
+ "%d", GIT_REPO_VERSION);
git_config_set("core.repositoryformatversion", repo_version_string);
 
path[len] = 0;
@@ -414,13 +415,13 @@ int init_db(const char *template_dir, unsigned int flags)
 */
if (shared_repository < 0)
/* force to the mode value */
-   sprintf(buf, "0%o", -shared_repository);
+   xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);
else if (shared_repository == PERM_GROUP)
-   sprintf(buf, "%d", OLD_PERM_GROUP);
+   xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
else if (shared_repository == PERM_EVERYBODY)
-   sprintf(buf, "%d", OLD_PERM_EVERYBODY);
+   xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
else
-   die("oops");
+   die("BUG: invalid value for shared_repository");
git_config_set("core.sharedrepository", buf);
git_config_set("receive.denyNonFastforwards", "true");
}
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 3b04a0f..0e30d86 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -96,12 +96,13 @@ static int show_tree(const unsigned char *sha1, struct 
strbuf *base,
if (!strcmp(type, blob_type)) {
unsigned long size;
if (sha1_object_info(sha1, ) == OBJ_BAD)
-   strcpy(size_text, "BAD");
+   xsnprintf(size_text, sizeof(size_text),
+

Re: [PATCH 15/67] convert trivial sprintf / strcpy calls to xsnprintf

[Stefan Beller]

On Tue, Sep 15, 2015 at 11:42 AM, Jeff King  wrote:
> That misses the "assert" behavior of xsnprintf.

When I saw the first patches of this series, I like them.

Some off topic thoughts:

Having an "assert" behavior is not a good user experience though
and should be fixed. To fix it we need stack traces. And the git
version. And telling the user to send it to the mailing list.

I wonder if we want to include a trace where possible (i.e.
when compiled with gcc or other precompiler conditions)
into these assertive behaviors.
I'd guess we have an assertive behavior if die("BUG:...") is called,
so maybe we can just check inside of die if we want to print
a stack trace additionally ?

In my dream world we would have a similar mechanism as in
the kernel, a "BUG:..." will be automatically sent to some
collection agency via UDP.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html