threads / patch / 9277

patchDon't rely on unspecified behavior

Subject: [PATCH] Don't rely on unspecified behavior

## tl;dr

11 messages between Jul 28, 2007 and Jul 28, 2007. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Thomas Schwinge· Jul 28, 2007, 16:39 UTC · lore

[PATCH] Support building on GNU/Hurd

GNU/Hurd systems don't have strlcpy either.
Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
---
 Makefile |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to Makefile +4 −0
diff --git a/Makefile b/Makefile
index 2fea115..8d9a01b 100644
--- a/Makefile
+++ b/Makefile
@@ -458,6 +458,10 @@ ifeq ($(uname_S),AIX)
 	NO_STRLCPY = YesPlease
 	NEEDS_LIBICONV=YesPlease
 endif
+ifeq ($(uname_S),GNU)
+	# GNU/Hurd
+	NO_STRLCPY=YesPlease
+endif
 ifeq ($(uname_S),IRIX64)
 	NO_IPV6=YesPlease
 	NO_SETENV=YesPlease

-- 
1.5.3.rc3.26.g6c58-dirty
Thomas Schwinge· Jul 28, 2007, 16:39 UTC · re: Thomas Schwinge · lore

Calling access(p, m) with p == NULL is not specified, so don't do that. On GNU/Hurd systems doing so will result in an SIGSEGV.

Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
---
 builtin-add.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-add.c +1 −1
diff --git a/builtin-add.c b/builtin-add.c
index 5e6748f..c13c738 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -74,7 +74,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec)
 	path = git_path("info/exclude");
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (!access(excludes_file, R_OK))
+	if (excludes_file != NULL && !access(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 
 	/*

-- 
1.5.3.rc3.26.g6c58-dirty
Thomas Glanzmann· Jul 28, 2007, 17:39 UTC · re: Thomas Schwinge · lore

Re: [PATCH] Don't rely on unspecified behavior

Hello,
> Calling access(p, m) with p == NULL is not specified, so don't do
> that.  On GNU/Hurd systems doing so will result in an SIGSEGV.

a friend of mine choked on this one when tried git for the second time (the first time "git-repack -a -d -f" screwed his repository after the initial checkout. This is fixed for a long time). Lucky me that he had his libusbdriver in LD_PRELOAD which could not handle the NULL argument. And I always thought libc would make the check before it does the system call or does GNU/hurts not use the gnu libc?

	Thomas
Thomas Schwinge· Jul 28, 2007, 18:25 UTC · re: Thomas Glanzmann · lore

Re: [PATCH] Don't rely on unspecified behavior

Hello!
On Sat, Jul 28, 2007 at 07:39:48PM +0200, Thomas Glanzmann wrote:
Show 9 quoted lines
> > Calling access(p, m) with p == NULL is not specified, so don't do
> > that.  On GNU/Hurd systems doing so will result in an SIGSEGV.
> 
> a friend of mine choked on this one when tried git for the second time
> (the first time "git-repack -a -d -f" screwed his repository after the
> initial checkout. This is fixed for a long time). Lucky me that he had
> his libusbdriver in LD_PRELOAD which could not handle the NULL argument.
> And I always thought libc would make the check before it does the system
> call or does GNU/hurts not use the gnu libc?

GNU/Hurd systems do (obviously ;-) use the GNU libc. The glibc maintainer Roland McGrath explicitly told me that ``access (NULL, m)'' shall not be caught as it is not specified and thus must not be invoked like this.

I noticed that the patch I sent was prepared for an old version of the file. I'll send an updated patch that applies to the current revision.

Regards,
 Thomas
Thomas Schwinge· Jul 28, 2007, 18:26 UTC · re: Thomas Glanzmann · lore

Calling access(p, m) with p == NULL is not specified, so don't do that. On GNU/Hurd systems doing so will result in a SIGSEGV.

Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
---
 builtin-add.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-add.c +1 −1
diff --git a/builtin-add.c b/builtin-add.c
index 7345479..de5c108 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -60,7 +60,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
 		path = git_path("info/exclude");
 		if (!access(path, R_OK))
 			add_excludes_from_file(dir, path);
-		if (!access(excludes_file, R_OK))
+		if (excludes_file != NULL && !access(excludes_file, R_OK))
 			add_excludes_from_file(dir, excludes_file);
 	}
 

-- 
1.5.3.rc3.26.g6c58-dirty
Johannes Schindelin· Jul 28, 2007, 19:30 UTC · re: Thomas Schwinge · lore

Re: [PATCH] Don't rely on unspecified behavior

Hi,
On Sat, 28 Jul 2007, Thomas Schwinge wrote:
Show 5 quoted lines
> Calling access(p, m) with p == NULL is not specified, so don't do that.  On
> GNU/Hurd systems doing so will result in a SIGSEGV.
> 
> Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
> ---
Isn't this the same patch as you sent before?
Show 13 quoted lines
>  builtin-add.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-add.c b/builtin-add.c
> index 7345479..de5c108 100644
> --- a/builtin-add.c
> +++ b/builtin-add.c
> @@ -60,7 +60,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
>  		path = git_path("info/exclude");
>  		if (!access(path, R_OK))
>  			add_excludes_from_file(dir, path);
> -		if (!access(excludes_file, R_OK))
> +		if (excludes_file != NULL && !access(excludes_file, R_OK))
We usually omit the "!= NULL"; see the other source code in git.git.

Ciao, Dscho

Thomas Glanzmann· Jul 28, 2007, 19:34 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Don't rely on unspecified behavior

Hello Dscho,
> Isn't this the same patch as you sent before?
> > @@ -74,7 +74,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec)
> > @@ -60,7 +60,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
       ~~~~~ ~~~~~                                                                            ~

The offset of the diff has changed. Not that git couldn't sort it out by itself. And the function had one or more parameters less.

	Thomas
Johannes Schindelin· Jul 28, 2007, 20:16 UTC · re: Thomas Glanzmann · lore

Re: [PATCH] Don't rely on unspecified behavior

Hi,
On Sat, 28 Jul 2007, Thomas Glanzmann wrote:
Show 8 quoted lines
> > Isn't this the same patch as you sent before?
> 
> > > @@ -74,7 +74,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec)
> > > @@ -60,7 +60,7 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
>        ~~~~~ ~~~~~                                                                            ~
> 
> The offset of the diff has changed. Not that git couldn't sort it out by
> itself. And the function had one or more parameters less.
Ah.  Thanks for the explanation.

Ciao, Dscho

Thomas Schwinge· Jul 28, 2007, 19:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Don't rely on unspecified behavior

Hello!
On Sat, Jul 28, 2007 at 08:30:07PM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
> On Sat, 28 Jul 2007, Thomas Schwinge wrote:
> > Calling access(p, m) with p == NULL is not specified, so don't do that.  On
> > GNU/Hurd systems doing so will result in a SIGSEGV.
> > 
> > Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
> > ---
> 
> Isn't this the same patch as you sent before?

As I wrote in <20070728182542.GA22651@fencepost.gnu.org>: ``I noticed that the patch I sent was prepared for an old version of the file. I'll send an updated patch that applies to the current revision.''

> > +		if (excludes_file != NULL && !access(excludes_file, R_OK))
> 
> We usually omit the "!= NULL"; see the other source code in git.git.
Okay, so I should sent a thusly modified version to get it applied?
Regards,
 Thomas
Johannes Schindelin· Jul 28, 2007, 20:17 UTC · re: Thomas Schwinge · lore

Re: [PATCH] Don't rely on unspecified behavior

Hi,
On Sat, 28 Jul 2007, Thomas Schwinge wrote:
Show 13 quoted lines
> On Sat, Jul 28, 2007 at 08:30:07PM +0100, Johannes Schindelin wrote:
> > On Sat, 28 Jul 2007, Thomas Schwinge wrote:
> > > Calling access(p, m) with p == NULL is not specified, so don't do that.  On
> > > GNU/Hurd systems doing so will result in a SIGSEGV.
> > > 
> > > Signed-off-by: Thomas Schwinge <tschwinge@gnu.org>
> > > ---
> > 
> > Isn't this the same patch as you sent before?
> 
> As I wrote in <20070728182542.GA22651@fencepost.gnu.org>: ``I noticed
> that the patch I sent was prepared for an old version of the file.  I'll
> send an updated patch that applies to the current revision.''
Ah.
Show 5 quoted lines
> > > +		if (excludes_file != NULL && !access(excludes_file, R_OK))
> > 
> > We usually omit the "!= NULL"; see the other source code in git.git.
> 
> Okay, so I should sent a thusly modified version to get it applied?

I don't think that is necessary; a small change like this is usually fixed by Junio with --amend.

Ciao, Dscho

← back to recent threads