Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 04:32:41PM +0100, Ramsay Jones wrote:

> >>char buf[48];
> >> -  int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
> >> +  int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
> >>   report, sha1_to_hex(sha1));
> >>write_or_die(1, buf, len);
> > 
> > So it's pretty unclear here whether that 48 is big enough (it is, if you
> > read the whole function, because "report" is always a 4-char string).
> > Yuck. At least there should be a comment explaining why 48 is big
> > enough.
> 
> Agreed, again I would use something like:
> 
>   char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 
> (\t\n\0) */

Yes, that's much better, I think.

> (and yes yuck - is report ever likely to increase? "bitmap" perhaps?)

It shouldn't. It's easy to think that's a filetype, but it really is
just "did you tell me --keep". TBH, I am not really sure that switching
it accomplishes anything.

-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 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Ramsay Jones]

On 03/06/16 09:53, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:16AM +, Elia Pinto wrote:
> 
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index e8c71fc..c032fe7 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const 
>> char *curr_pack_name,
>>  printf("%s\n", sha1_to_hex(sha1));
>>  } else {
>>  char buf[48];
>> -int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
>> +int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
>> report, sha1_to_hex(sha1));
>>  write_or_die(1, buf, len);
> 
> So it's pretty unclear here whether that 48 is big enough (it is, if you
> read the whole function, because "report" is always a 4-char string).
> Yuck. At least there should be a comment explaining why 48 is big
> enough.

Agreed, again I would use something like:

char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 
(\t\n\0) */

(and yes yuck - is report ever likely to increase? "bitmap" perhaps?)

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 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the 
call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining 
calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft 
error
have not been changed.

Signed-off-by: Elia Pinto 
---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e8c71fc..c032fe7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char 
*curr_pack_name,
printf("%s\n", sha1_to_hex(sha1));
} else {
char buf[48];
-   int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
+   int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
   report, sha1_to_hex(sha1));
write_or_die(1, buf, len);
 
-- 
2.9.0.rc1.265.geb5d750

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