[PATCH] symfile.c, find_separate_debug_file

[PATCH] symfile.c, find_separate_debug_file

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

Just for a change, here's one that was not inspired by Coverity.

Find separate debug file does not work in the case where the path
lies in the global debug file directory -- because we stick an extra
slash into the pathname.


[-- Attachment #2: debug-file-dir.txt --]
[-- Type: text/plain, Size: 996 bytes --]

2007-08-02  Michael Snyder  <msnyder@access-company.com>

	* symfile.c (find_separate_debug_file): While consing up a path, 
	don't stick in an extra DIR_SEPARATOR.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.189
diff -p -r1.189 symfile.c
*** symfile.c	31 Jul 2007 21:42:19 -0000	1.189
--- symfile.c	3 Aug 2007 01:10:42 -0000
*************** find_separate_debug_file (struct objfile
*** 1355,1361 ****
  
    /* Then try in the global debugfile directory.  */
    strcpy (debugfile, debug_file_directory);
!   strcat (debugfile, "/");
    strcat (debugfile, dir);
    strcat (debugfile, basename);
  
--- 1355,1364 ----
  
    /* Then try in the global debugfile directory.  */
    strcpy (debugfile, debug_file_directory);
! 
!   if (debugfile[strlen (debugfile) - 1] != '/' && dir[0] != '/')
!     strcat (debugfile, "/");
! 
    strcat (debugfile, dir);
    strcat (debugfile, basename);
  

Re: [PATCH] symfile.c, find_separate_debug_file

[Daniel Jacobowitz]

On Thu, Aug 02, 2007 at 06:14:44PM -0700, Michael Snyder wrote:
> Just for a change, here's one that was not inspired by Coverity.
> 
> Find separate debug file does not work in the case where the path
> lies in the global debug file directory -- because we stick an extra
> slash into the pathname.

Why doesn't it work with an extra slash?  It shouldn't make a
difference.  Anyway:

> 	* symfile.c (find_separate_debug_file): While consing up a path, 
> 	don't stick in an extra DIR_SEPARATOR.

That's right, use DIR_SEPARATOR :-)  IS_DIR_SEPARATOR in this case, I
think?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

> On Thu, Aug 02, 2007 at 06:14:44PM -0700, Michael Snyder wrote:
> > Just for a change, here's one that was not inspired by Coverity.
> >
> > Find separate debug file does not work in the case where the path
> > lies in the global debug file directory -- because we stick an extra
> > slash into the pathname.
>
> Why doesn't it work with an extra slash?  It shouldn't make a
> difference.  Anyway:
>
> > * symfile.c (find_separate_debug_file): While consing up a path,
> > don't stick in an extra DIR_SEPARATOR.
>
> That's right, use DIR_SEPARATOR :-)  IS_DIR_SEPARATOR in this case, I
> think?

The existing code doesn't use it.  If you want me to undertake to
revise the code to use DIR_SEPARATOR, I'll be glad to do it.

But it shouldn't hold up this patch.

Re: [PATCH] symfile.c, find_separate_debug_file

[Daniel Jacobowitz]

On Thu, Aug 02, 2007 at 09:36:27PM -0700, Michael Snyder wrote:
> The existing code doesn't use it.  If you want me to undertake to
> revise the code to use DIR_SEPARATOR, I'll be glad to do it.

  for (i = strlen(dir) - 1; i >= 0; i--)
    {
      if (IS_DIR_SEPARATOR (dir[i]))
        break;
    }
  gdb_assert (i >= 0 && IS_DIR_SEPARATOR (dir[i]));

...

  if (canon_name
      && strncmp (canon_name, gdb_sysroot, strlen (gdb_sysroot)) == 0
      && IS_DIR_SEPARATOR (canon_name[strlen (gdb_sysroot)]))

Adding "/" to strings is fine; it's when looking at passed in strings
that you have to check directory separators more carefully.

On Fri, Aug 03, 2007 at 12:10:30AM -0700, Michael Snyder wrote:
> Well I don't know -- they seemed to hurt when I tried it,
> and getting rid of them certainly seemed to help.
> 
> If I type /usr/lib//tmp/foo, isn't that equivalent, at least
> in some contexts, to typing /tmp/foo?

To my knowledge this is true only in Emacs, not in any normal
operating system's file access routines.  GDB does not run on top of
Emacs...

Maybe you could describe how it seemed to hurt.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

> Adding "/" to strings is fine; it's when looking at passed in strings
> that you have to check directory separators more carefully.
>
> On Fri, Aug 03, 2007 at 12:10:30AM -0700, Michael Snyder wrote:
> > Well I don't know -- they seemed to hurt when I tried it,
> > and getting rid of them certainly seemed to help.
> >
> > If I type /usr/lib//tmp/foo, isn't that equivalent, at least
> > in some contexts, to typing /tmp/foo?
>
> To my knowledge this is true only in Emacs, not in any normal
> operating system's file access routines.  GDB does not run on top of
> Emacs...
>
> Maybe you could describe how it seemed to hurt.

Sure.  It generates a string such as
"/usr/local/lib/debug//opt/grbx/lib.so",
which is then passed to the libc function 'open', which fails to find the
file.

When I remove the extra slash, open succeeds.

This is glibc 2.5 running under UML in Scratchbox, on Ubuntu.
Gdb is x86-cross-x86, not that that's probably relevant.

Re: [PATCH] symfile.c, find_separate_debug_file

[Daniel Jacobowitz]

On Fri, Aug 03, 2007 at 07:41:56AM -0700, Michael Snyder wrote:
> Sure.  It generates a string such as
> "/usr/local/lib/debug//opt/grbx/lib.so",
> which is then passed to the libc function 'open', which fails to find the
> file.
> 
> When I remove the extra slash, open succeeds.
> 
> This is glibc 2.5 running under UML in Scratchbox, on Ubuntu.
> Gdb is x86-cross-x86, not that that's probably relevant.

That is strange.  If you try it outside of UML and scratchbox, I bet
that won't happen; my money is on a UML bug in the host filesystem
driver.

I can't remember where in POSIX it is, and I can't find it, but I'm
pretty sure that behavior is non-conforming.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

----- Original Message ----- 
From: "Daniel Jacobowitz" <drow@false.org>
To: "Michael Snyder" <msnyder@sonic.net>
Cc: <gdb-patches@sourceware.org>; "Mark Kettenis" <mark.kettenis@xs4all.nl>
Sent: Friday, August 03, 2007 8:00 AM
Subject: Re: [PATCH] symfile.c, find_separate_debug_file


> On Fri, Aug 03, 2007 at 07:41:56AM -0700, Michael Snyder wrote:
> > Sure.  It generates a string such as
> > "/usr/local/lib/debug//opt/grbx/lib.so",
> > which is then passed to the libc function 'open', which fails to find
the
> > file.
> >
> > When I remove the extra slash, open succeeds.
> >
> > This is glibc 2.5 running under UML in Scratchbox, on Ubuntu.
> > Gdb is x86-cross-x86, not that that's probably relevant.
>
> That is strange.  If you try it outside of UML and scratchbox, I bet
> that won't happen; my money is on a UML bug in the host filesystem
> driver.
>
> I can't remember where in POSIX it is, and I can't find it, but I'm
> pretty sure that behavior is non-conforming.

OK, well I'm interested to find out.  I *think* I saw the same failure
under Ubuntu, but I'll have to confirm when I get back to the office.

Either way, though, there's nothing wrong with taking out an extra
slash, is there?  It doesn't serve any purpose.  Maybe gdb should
work even if the underlying OS isn't Posix compliant.

Re: [PATCH] symfile.c, find_separate_debug_file

[Daniel Jacobowitz]

On Fri, Aug 03, 2007 at 08:04:25AM -0700, Michael Snyder wrote:
> Either way, though, there's nothing wrong with taking out an extra
> slash, is there?  It doesn't serve any purpose.

No objection.  It's ugly.

> Maybe gdb should
> work even if the underlying OS isn't Posix compliant.

Not going there...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] symfile.c, find_separate_debug_file

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> I can't remember where in POSIX it is, and I can't find it, but I'm
> pretty sure that behavior is non-conforming.

Base definitions, 3.266 Pathname: ... Multiple successive slashes are
considered to be the same as one slash.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] symfile.c, find_separate_debug_file

[Eli Zaretskii]

> From: "Michael Snyder" <msnyder@sonic.net>
> Cc: <gdb-patches@sourceware.org>
> Date: Thu, 2 Aug 2007 21:36:27 -0700
> 
> > > * symfile.c (find_separate_debug_file): While consing up a path,
> > > don't stick in an extra DIR_SEPARATOR.
> >
> > That's right, use DIR_SEPARATOR :-)  IS_DIR_SEPARATOR in this case, I
> > think?
> 
> The existing code doesn't use it.

Correction: the existing code appends "/" unconditionally, which is
fine, since both Posix and non-Posix platforms support forward slashes
in file names.

But Daniel didn't mean that: he meant to use IS_DIR_SEPARATOR instead
of literal comparison to '/' alone.  The existing code didn't test
anything, so it didn't have to cope with this issue.

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

----- Original Message ----- 
From: "Eli Zaretskii" <eliz@gnu.org>
To: "Michael Snyder" <msnyder@sonic.net>
Cc: <drow@false.org>; <gdb-patches@sourceware.org>
Sent: Friday, August 03, 2007 5:31 AM
Subject: Re: [PATCH] symfile.c, find_separate_debug_file


> > From: "Michael Snyder" <msnyder@sonic.net>
> > Cc: <gdb-patches@sourceware.org>
> > Date: Thu, 2 Aug 2007 21:36:27 -0700
> >
> > > > * symfile.c (find_separate_debug_file): While consing up a path,
> > > > don't stick in an extra DIR_SEPARATOR.
> > >
> > > That's right, use DIR_SEPARATOR :-)  IS_DIR_SEPARATOR in this case, I
> > > think?
> >
> > The existing code doesn't use it.
>
> Correction: the existing code appends "/" unconditionally, which is
> fine, since both Posix and non-Posix platforms support forward slashes
> in file names.
>
> But Daniel didn't mean that: he meant to use IS_DIR_SEPARATOR instead
> of literal comparison to '/' alone.  The existing code didn't test
> anything, so it didn't have to cope with this issue.

Yes, I understood exactly what Daniel meant.  I was saying,
the existing code uses the literal '/' instead of the symbolic
DIR_SEPARATOR.

I offered to have another go at the code and clean up the literals,
but as a separate patch to this one.

Re: [PATCH] symfile.c, find_separate_debug_file

[Daniel Jacobowitz]

On Fri, Aug 03, 2007 at 07:37:35AM -0700, Michael Snyder wrote:
> Yes, I understood exactly what Daniel meant.  I was saying,
> the existing code uses the literal '/' instead of the symbolic
> DIR_SEPARATOR.
> 
> I offered to have another go at the code and clean up the literals,
> but as a separate patch to this one.

You added two lines:

!   if (debugfile[strlen (debugfile) - 1] != '/' && dir[0] != '/')
!     strcat (debugfile, "/");

I think you're talking about the second one, which adds a "/".  That's
fine, but the first line isn't.  Other places in that function which
compare against '/' use IS_DIR_SEPARATOR already.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] symfile.c, find_separate_debug_file

[Eli Zaretskii]

> From: "Michael Snyder" <msnyder@sonic.net>
> Cc: <drow@false.org>, <gdb-patches@sourceware.org>
> Date: Fri, 3 Aug 2007 07:37:35 -0700
> 
> Yes, I understood exactly what Daniel meant.  I was saying,
> the existing code uses the literal '/' instead of the symbolic
> DIR_SEPARATOR.

I have nothing against appending a literal "/", but comparing against
a literal '/' should be done via IS_DIR_SEPARATOR.

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

----- Original Message ----- 
From: "Eli Zaretskii" <eliz@gnu.org>
To: "Michael Snyder" <msnyder@sonic.net>
Cc: <drow@false.org>; <gdb-patches@sourceware.org>
Sent: Friday, August 03, 2007 10:11 AM
Subject: Re: [PATCH] symfile.c, find_separate_debug_file


> > From: "Michael Snyder" <msnyder@sonic.net>
> > Cc: <drow@false.org>, <gdb-patches@sourceware.org>
> > Date: Fri, 3 Aug 2007 07:37:35 -0700
> >
> > Yes, I understood exactly what Daniel meant.  I was saying,
> > the existing code uses the literal '/' instead of the symbolic
> > DIR_SEPARATOR.
>
> I have nothing against appending a literal "/", but comparing against
> a literal '/' should be done via IS_DIR_SEPARATOR.

Right, I just agreed to that.

Re: [PATCH] symfile.c, find_separate_debug_file

[Mark Kettenis]

> Date: Thu, 2 Aug 2007 18:14:44 -0700 (PDT)
> From: msnyder@sonic.net
> 
> Just for a change, here's one that was not inspired by Coverity.
> 
> Find separate debug file does not work in the case where the path
> lies in the global debug file directory -- because we stick an extra
> slash into the pathname.

Since when do extra slashes in a pathname hurt?

> 2007-08-02  Michael Snyder  <msnyder@access-company.com>
> 
> 	* symfile.c (find_separate_debug_file): While consing up a path, 
> 	don't stick in an extra DIR_SEPARATOR.

Re: [PATCH] symfile.c, find_separate_debug_file

[Michael Snyder]

> > Date: Thu, 2 Aug 2007 18:14:44 -0700 (PDT)
> > From: msnyder@sonic.net
> >
> > Just for a change, here's one that was not inspired by Coverity.
> >
> > Find separate debug file does not work in the case where the path
> > lies in the global debug file directory -- because we stick an extra
> > slash into the pathname.
>
> Since when do extra slashes in a pathname hurt?

Well I don't know -- they seemed to hurt when I tried it,
and getting rid of them certainly seemed to help.

If I type /usr/lib//tmp/foo, isn't that equivalent, at least
in some contexts, to typing /tmp/foo?