threads / patch / 18221

patchgrep: make show_line more portable

Subject: [PATCH] grep: make show_line more portable

## tl;dr

8 messages between Mar 9, 2009 and Mar 9, 2009. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Brian Gernhardt· Mar 9, 2009, 01:15 UTC · lore

On OS X the printf specifier "%.0s" outputs the entire string instead of 0 characters as POSIX states.

In addition, for * width or precision printf expects an integer argument. On systems were regoff_t is 64-bit, unexpected results can occur.

To fix these, use if statements to catch 0 precisions and casts to convert regoff_t to int.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---
 grep.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)
Show changes to grep.c +10 −6
diff --git a/grep.c b/grep.c
index cace1c8..ec68200 100644
--- a/grep.c
+++ b/grep.c
@@ -489,18 +489,22 @@ 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,
-			       opt->color_match,
-			       match.rm_eo - match.rm_so, bol + match.rm_so,
-			       GIT_COLOR_RESET);
+			if( match.rm_so > 0 )
+				printf( "%.*s", (int) match.rm_so, bol );
+			if( match.rm_eo > match.rm_so )
+				printf("%s%.*s%s",
+					   opt->color_match,
+					  (int) (match.rm_eo - match.rm_so), bol + match.rm_so,
+					   GIT_COLOR_RESET);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
 			eflags = REG_NOTBOL;
 		}
 		*eol = ch;
 	}
-	printf("%.*s\n", rest, bol);
+	if( rest > 0 )
+		printf("%.*s", rest, bol);
+	printf("\n");
 }
 
 static int grep_buffer_1(struct grep_opt *opt, const char *name,

-- 
1.6.2.222.g01cbd
Junio C Hamano· Mar 9, 2009, 01:35 UTC · re: Brian Gernhardt · lore

Re: [PATCH] grep: make show_line more portable

Brian Gernhardt <benji@silverinsanity.com> writes:
Show 6 quoted lines
> On OS X the printf specifier "%.0s" outputs the entire string instead
> of 0 characters as POSIX states.
>
> In addition, for * width or precision printf expects an integer
> argument.  On systems were regoff_t is 64-bit, unexpected results can
> occur.
I would prefer to see these two issues solved as separate issues.

Specifically, I'd like to know if the patch from me to you a few message ago solves the issue.

If you still need a "some implementations of printf is broken with respect to 0 precision" workaround on top of that patch, we would want to add it separately, but it may have to cover not just this printf(), as I am not convinced this is the only place that lets (integer) 0 passed to the "%.*s" format. That patch needs to be written after a separate auditing of output from "git grep -n -e 'printf.*%\.\*s'", which I do not think happened yet (at least I haven't done that, and I somehow do not think you have yet either).

Jay Soffian· Mar 9, 2009, 02:22 UTC · re: Brian Gernhardt · lore

Re: [PATCH] grep: make show_line more portable

On Sun, Mar 8, 2009 at 9:15 PM, Brian Gernhardt <benji@silverinsanity.com> wrote:

> On OS X the printf specifier "%.0s" outputs the entire string instead
> of 0 characters as POSIX states.
Does not reproduce for me:
$ cat foo.c && gcc -m64 foo.c -o foo32 && gcc foo.c -o foo64 && file
foo32 foo64 && ./foo32 && ./foo64
#include "stdio.h"
#include "stdlib.h"
main() {
	printf("1 '%.0s'\n", "foobar");
	printf("2 '%.*s'\n", 0, "foobar");
	exit(0);
}
foo32: Mach-O 64-bit executable x86_64
foo64: Mach-O executable i386
1 ''
2 ''
1 ''
2 ''
OS X 10.5.6 (Darwin 9.6.0). i686-apple-darwin9-gcc-4.0.1. Same linkage for both:

/usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 111.1.3)

j.
Jay Soffian· Mar 9, 2009, 02:23 UTC · re: Jay Soffian · lore

Re: [PATCH] grep: make show_line more portable

On Sun, Mar 8, 2009 at 10:22 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> foo32: Mach-O 64-bit executable x86_64
> foo64: Mach-O executable i386

Okay, so I may be brain-damaged in my naming, but that doesn't invalidate the results. :-)

j.
Brian Gernhardt· Mar 9, 2009, 02:44 UTC · re: Jay Soffian · lore

Re: [PATCH] grep: make show_line more portable

On Mar 8, 2009, at 10:22 PM, Jay Soffian wrote:
Show 6 quoted lines
> On Sun, Mar 8, 2009 at 9:15 PM, Brian Gernhardt
> <benji@silverinsanity.com> wrote:
>> On OS X the printf specifier "%.0s" outputs the entire string instead
>> of 0 characters as POSIX states.
>
> Does not reproduce for me:

Nor for me, as I noted on the other thread... And looking again, I was reading the man page for printf(1), not printf(3). Ouch. *grumble, grumble* I'm crawling back under my rock now.

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

Re: [PATCH] grep: make show_line more portable

Brian Gernhardt <benji@silverinsanity.com> writes:
Show 12 quoted lines
> On Mar 8, 2009, at 10:22 PM, Jay Soffian wrote:
>
>> On Sun, Mar 8, 2009 at 9:15 PM, Brian Gernhardt
>> <benji@silverinsanity.com> wrote:
>>> On OS X the printf specifier "%.0s" outputs the entire string instead
>>> of 0 characters as POSIX states.
>>
>> Does not reproduce for me:
>
> Nor for me, as I noted on the other thread...  And looking again, I
> was reading the man page for printf(1), not printf(3).  Ouch.
> *grumble, grumble*  I'm crawling back under my rock now.

Heh, people make mistakes and others are here to help spot them. Collectively we all win.

Thanks for a breakage report, initial fix and a confirmation.
Johannes Schindelin· Mar 9, 2009, 09:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] grep: make show_line more portable

Hi,
On Sun, 8 Mar 2009, Junio C Hamano wrote:
Show 17 quoted lines
> Brian Gernhardt <benji@silverinsanity.com> writes:
> 
> > On Mar 8, 2009, at 10:22 PM, Jay Soffian wrote:
> >
> >> On Sun, Mar 8, 2009 at 9:15 PM, Brian Gernhardt
> >> <benji@silverinsanity.com> wrote:
> >>> On OS X the printf specifier "%.0s" outputs the entire string instead
> >>> of 0 characters as POSIX states.
> >>
> >> Does not reproduce for me:
> >
> > Nor for me, as I noted on the other thread...  And looking again, I
> > was reading the man page for printf(1), not printf(3).  Ouch.
> > *grumble, grumble*  I'm crawling back under my rock now.
> 
> Heh, people make mistakes and others are here to help spot them.
> Collectively we all win.
One of my favorite quotes these days:

The computer "doth make fools of us all," so that any fool without the ability to share a laugh on himself will be unable to tolerate programming for long. ''(Gerald M. Weinberg)''

> Thanks for a breakage report, initial fix and a confirmation.

Yes, I think this discussion was valuable, not only because it fixed a bug, but also because I learnt that %.*s with a negative length defaults to the total string.

Ciao, Dscho

René Scharfe· Mar 9, 2009, 19:34 UTC · re: Brian Gernhardt · lore

Re: [PATCH] grep: make show_line more portable

Brian Gernhardt schrieb:
Show 13 quoted lines
> 
> On Mar 8, 2009, at 10:22 PM, Jay Soffian wrote:
> 
>> On Sun, Mar 8, 2009 at 9:15 PM, Brian Gernhardt
>> <benji@silverinsanity.com> wrote:
>>> On OS X the printf specifier "%.0s" outputs the entire string instead
>>> of 0 characters as POSIX states.
>>
>> Does not reproduce for me:
> 
> Nor for me, as I noted on the other thread...  And looking again, I was
> reading the man page for printf(1), not printf(3).  Ouch.  *grumble,
> grumble*  I'm crawling back under my rock now.

Sorry for introducing a Linuxism. :-/ Thanks for testing and reporting and for fixing the bug.

René

← back to recent threads