git-grep Bus Error

git-grep Bus Error

[Brian Gernhardt]

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

> 			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

Re: git-grep Bus Error

[Sam Hocevar]

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.

-- 
Sam.

Re: git-grep Bus Error

[Brian Gernhardt]

On Mar 8, 2009, at 7:41 PM, Sam Hocevar wrote:

> 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

Re: git-grep Bus Error

[Junio C Hamano]

Sam Hocevar <sam@zoy.org> writes:

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

Re: git-grep Bus Error

[Junio C Hamano]

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

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

Re: git-grep Bus Error

[Brian Gernhardt]

On Mar 8, 2009, at 9:11 PM, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

Re: git-grep Bus Error

[Brian Gernhardt]

On Mar 8, 2009, at 9:11 PM, Junio C Hamano wrote:

> 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

Re: git-grep Bus Error

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

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

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