threads / discuss / 18217

git-grep Bus Error

Subject: git-grep Bus Error

## tl;dr

9 messages between Mar 8, 2009 and Mar 9, 2009.

replies: 8people: 4as markdown or json

Brian Gernhardt· Mar 8, 2009, 23:27 UTC · lore

The --color display code in git-grep is giving me a bus error in show_line at line 492:

Show 6 quoted lines
>                         printf("%.*s%s%.*s%s",
>                                match.rm_so, bol,
>                                opt->color_match,
>                                match.rm_eo - match.rm_so, bol +  
> match.rm_so,
>                                GIT_COLOR_RESET);

The first problem is that %.*s does not appear to do on OS X what the author thinks it does. A precision of 0 for %s is listed in "man printf" as printing the entire string.

To fix that, I changed it to the following:
Show 7 quoted lines
> 			if( match.rm_so > 0 )
> 				printf( "%.*s", match.rm_so, bol );
> 			if( match.rm_eo > match.rm_so )
> 				printf("%s%.*s%s",
> 					   opt->color_match,
> 					   match.rm_eo - match.rm_so, bol + match.rm_so,
> 					   GIT_COLOR_RESET);

This code does not fail, but instead gives lines like the following (showing the raw color codes):

.gitignore:\033[31m\033[1m(nugit
GIT_COLOR_RESET is apparently being ignored, and I don't know why.

Adding a line to check the values of rm_so, rm_eo, and the difference between the two gives:

> 			printf( "%d %d %d",
> 				  match.rm_so, match.rm_eo,
> 				  match.rm_eo - match.rm_so );

.gitignore:0 0 3\033[31m\033[1m(nugit .mailmap:23 0 26(null)\033[31m\033[1m(nugit-shortlog to fix a few botched name translations-shortlog to fix a few botched name translations

And now I'm baffled. Apparently my computer thinks 0 - 0 == 3 and 0 - 23 == 26.

Can I get some help?
~~ Brian
Sam Hocevar· Mar 8, 2009, 23:41 UTC · re: Brian Gernhardt · lore

Re: git-grep Bus Error

On Sun, Mar 08, 2009, Brian Gernhardt wrote:
Show 11 quoted lines
> >			printf( "%d %d %d",
> >				  match.rm_so, match.rm_eo,
> >				  match.rm_eo - match.rm_so );
> 
> .gitignore:0 0 3\033[31m\033[1m(nugit
> .mailmap:23 0 26(null)\033[31m\033[1m(nugit-shortlog to fix a few  
> botched name translations-shortlog to fix a few botched name  
> translations
> 
> And now I'm baffled.  Apparently my computer thinks 0 - 0 == 3 and 0 -  
> 23 == 26.
   rm_so and rm_eo are ints on Linux but off_t's on Darwin, hence
probably int64_t's here. You should cast the arguments.
-- 
Sam.
Brian Gernhardt· Mar 9, 2009, 00:58 UTC · re: Sam Hocevar · lore

Re: git-grep Bus Error

On Mar 8, 2009, at 7:41 PM, Sam Hocevar wrote:
Show 17 quoted lines
> On Sun, Mar 08, 2009, Brian Gernhardt wrote:
>
>>> 			printf( "%d %d %d",
>>> 				  match.rm_so, match.rm_eo,
>>> 				  match.rm_eo - match.rm_so );
>>
>> .gitignore:0 0 3\033[31m\033[1m(nugit
>> .mailmap:23 0 26(null)\033[31m\033[1m(nugit-shortlog to fix a few
>> botched name translations-shortlog to fix a few botched name
>> translations
>>
>> And now I'm baffled.  Apparently my computer thinks 0 - 0 == 3 and  
>> 0 -
>> 23 == 26.
>
>   rm_so and rm_eo are ints on Linux but off_t's on Darwin, hence
> probably int64_t's here. You should cast the arguments.

And that explains the warnings about the parameters to printf not being integers. I was looking at compat/regex/regex.h and was confused.

Adding a cast to int on all of the format specifiers solves my problems. Thank you.

~~ Brian
Junio C Hamano· Mar 9, 2009, 01:11 UTC · re: Sam Hocevar · lore

Re: git-grep Bus Error

Sam Hocevar <sam@zoy.org> writes:
Show 16 quoted lines
> On Sun, Mar 08, 2009, Brian Gernhardt wrote:
>
>> >			printf( "%d %d %d",
>> >				  match.rm_so, match.rm_eo,
>> >				  match.rm_eo - match.rm_so );
>> 
>> .gitignore:0 0 3\033[31m\033[1m(nugit
>> .mailmap:23 0 26(null)\033[31m\033[1m(nugit-shortlog to fix a few  
>> botched name translations-shortlog to fix a few botched name  
>> translations
>> 
>> And now I'm baffled.  Apparently my computer thinks 0 - 0 == 3 and 0 -  
>> 23 == 26.
>
>    rm_so and rm_eo are ints on Linux but off_t's on Darwin, hence
> probably int64_t's here. You should cast the arguments.

That is a very good point. In fact, "git grep -n -e 'printf.*%\.\*s'" reveals that many existing call sites to this form casts the precision argument explicitly to "int".

Brian, would this patch help?
 grep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index cace1c8..dcdbd5e 100644
--- a/grep.c
+++ b/grep.c
@@ -490,9 +490,9 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		*eol = '\0';
 		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
 			printf("%.*s%s%.*s%s",
-			       match.rm_so, bol,
+			       (int) match.rm_so, bol,
 			       opt->color_match,
-			       match.rm_eo - match.rm_so, bol + match.rm_so,
+			       (int)(match.rm_eo - match.rm_so), bol + match.rm_so,
 			       GIT_COLOR_RESET);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
Junio C Hamano· Mar 9, 2009, 01:18 UTC · re: Junio C Hamano · lore

Re: git-grep Bus Error

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Brian, would this patch help?
>
>  grep.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index cace1c8..dcdbd5e 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -490,9 +490,9 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
>  		*eol = '\0';
>  		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
>  			printf("%.*s%s%.*s%s",
> -			       match.rm_so, bol,
> +			       (int) match.rm_so, bol,
>  			       opt->color_match,
> -			       match.rm_eo - match.rm_so, bol + match.rm_so,
> +			       (int)(match.rm_eo - match.rm_so), bol + match.rm_so,
>  			       GIT_COLOR_RESET);
>  			bol += match.rm_eo;
>  			rest -= match.rm_eo;
I looked at all the hits from
    $ git grep -n -e 'printf.*%\.\*s' --and --not -e '(int)'
The above should be the only two places that need fixing.
Brian Gernhardt· Mar 9, 2009, 01:19 UTC · re: Junio C Hamano · lore

Re: git-grep Bus Error

On Mar 8, 2009, at 9:11 PM, Junio C Hamano wrote:
Show 10 quoted lines
> Sam Hocevar <sam@zoy.org> writes:
>
>>   rm_so and rm_eo are ints on Linux but off_t's on Darwin, hence
>> probably int64_t's here. You should cast the arguments.
>
> That is a very good point.  In fact, "git grep -n -e 'printf.*%\.\*s'"
> reveals that many existing call sites to this form casts the precision
> argument explicitly to "int".
>
> Brian, would this patch help?

Yes, except that the code also depends on printf("%.*s", 0, str) working properly. I just sent a patch which checks for zero width and performs the casts.

~~ Brian
Junio C Hamano· Mar 9, 2009, 01:24 UTC · re: Junio C Hamano · lore

[PATCH] grep: cast printf %.*s "precision" argument explicitly to int

On some systems, regoff_t that is the type of rm_so/rm_eo members are wider than int; %.*s precision specifier expects an int, so use an explicit cast.

A breakage reported on Darwin by Brian Gernhardt should be fixed with this patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Junio C Hamano <gitster@pobox.com> writes:
 > Brian, would this patch help?
 A resend with a commit log message.
 grep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index cace1c8..be99b34 100644
--- a/grep.c
+++ b/grep.c
@@ -490,9 +490,9 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		*eol = '\0';
 		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
 			printf("%.*s%s%.*s%s",
-			       match.rm_so, bol,
+			       (int)match.rm_so, bol,
 			       opt->color_match,
-			       match.rm_eo - match.rm_so, bol + match.rm_so,
+			       (int)(match.rm_eo - match.rm_so), bol + match.rm_so,
 			       GIT_COLOR_RESET);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;

-- 
1.6.2.206.g5bda76
Brian Gernhardt· Mar 9, 2009, 01:48 UTC · re: Junio C Hamano · lore

Re: git-grep Bus Error

On Mar 8, 2009, at 9:11 PM, Junio C Hamano wrote:
Show 22 quoted lines
> Brian, would this patch help?
>
> grep.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index cace1c8..dcdbd5e 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -490,9 +490,9 @@ static void show_line(struct grep_opt *opt, char  
> *bol, char *eol,
> 		*eol = '\0';
> 		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
> 			printf("%.*s%s%.*s%s",
> -			       match.rm_so, bol,
> +			       (int) match.rm_so, bol,
> 			       opt->color_match,
> -			       match.rm_eo - match.rm_so, bol + match.rm_so,
> +			       (int)(match.rm_eo - match.rm_so), bol + match.rm_so,
> 			       GIT_COLOR_RESET);
> 			bol += match.rm_eo;
> 			rest -= match.rm_eo;

Apparently so. Despite the fact that match.rm_so is 0 at times, "%.*s" works properly so the other half of the patch isn't needed. Odd.

~~ B
Junio C Hamano· Mar 9, 2009, 00:29 UTC · re: Brian Gernhardt · lore

Re: git-grep Bus Error

Brian Gernhardt <brian@gernhardtsoftware.com> writes:
Show 12 quoted lines
> The --color display code in git-grep is giving me a bus error in
> show_line at line 492:
>
>>                         printf("%.*s%s%.*s%s",
>>                                match.rm_so, bol,
>>                                opt->color_match,
>>                                match.rm_eo - match.rm_so, bol +
>> match.rm_so,
>>                                GIT_COLOR_RESET);
>
> The first problem is that %.*s does not appear to do on OS X what the
> author thinks it does.

Hmm, that means that printf on OSX does not dohat the POSIX thinks it ought to.

    http://www.opengroup.org/onlinepubs/009695399/functions/fprintf.html

says that "a negative precision is taken as if the precision were omitted"; it does not say "a negative or zero" here.

Which is a bit sad, because we would need to apply a workaround like yours. We shouldn't have to.

Show 9 quoted lines
> To fix that, I changed it to the following:
>
>> 			if( match.rm_so > 0 )
>> 				printf( "%.*s", match.rm_so, bol );
>> 			if( match.rm_eo > match.rm_so )
>> 				printf("%s%.*s%s",
>> 					   opt->color_match,
>> 					   match.rm_eo - match.rm_so, bol + match.rm_so,
>> 					   GIT_COLOR_RESET);
> This code does not fail, but instead gives lines like the following
> (showing the raw color codes):
>
> .gitignore:\033[31m\033[1m(nugit

Hmm, that is strange. Your above change issues color_match and COLOR_RESET only when you have something between rm_eo and rm_so. I do not see anything between "ESC [ 31 m" and "ESC [ m" above, and you have an extra "1" between "ESC [" and terminating "m" in the reset sequence.

← back to recent threads