[RFA] Should openp open directories?

[RFA] Should openp open directories?

[Joel Brobecker]

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

Hello,

Suppose you have a directory called "root", inside which you have a
directory called "foo". You also have an executable called "foo" in your
PATH that you would like to debug.

Suppose now that you launch gdb from "root" using the following command:
% gdb foo

Here is what is going to happen:
<<
GNU gdb 2002-04-17-cvs
[...]
This GDB was configured as "i686-pc-linux-gnu"..."/bonn.a/brobecke/gdb-public-ssh/gdb": not in executable format: Is a directory
>>

As you see, GDB found directory "foo" in the current directory, and
thought it was the executable it was asked to debug.

More generally, this problem can also happen if GDB finds in the PATH a
directory sharing the same name before the executable. For instance, say
your PATH is ~/bin:/usr/local/bin:/usr/bin, and that the directory
~/bin/sh exists, invoking GDB using the following command from your
home directory will also demonstrate the problem.
% gdb sh

This is because openp does not verify that the file it opens is a
regular file (ie not a directory, or a device, etc). From the
description of this function, and the name of the parameters, it seems
that this was the intent, and the attached patch fixes this.

2002-04-18  Joel Brobecker  <brobecker@gnat.com>

        * source.c (is_regular_file): New function.
        (openp): Check wether file to open is a regular file
        to avoid opening directories.

I verified that this patch does not introduce any regression:
Summary 1       2
FAIL    60      60
PASS    8242    8242
XFAIL   170     170
XPASS   11      11
Differences: 0

-- 
Joel

[-- Attachment #2: source.c.diff --]
[-- Type: text/plain, Size: 1719 bytes --]

Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.27
diff -c -3 -p -r1.27 source.c
*** source.c	12 Apr 2002 19:46:29 -0000	1.27
--- source.c	18 Apr 2002 10:23:55 -0000
*************** source_info (char *ignore, int from_tty)
*** 503,508 ****
--- 503,520 ----
  }
  \f
  
+ /* Return True if the file NAME exists and is a regular file */
+ static int
+ is_regular_file (const char *name)
+ {
+   struct stat st;
+   const int status = stat (name, &st);
+ 
+   if (status != 0)
+     return 0;
+ 
+   return S_ISREG (st.st_mode);
+ }
  
  /* Open a file named STRING, searching path PATH (dir names sep by some char)
     using mode MODE and protection bits PROT in the calls to open.
*************** openp (const char *path, int try_cwd_fir
*** 543,549 ****
    mode |= O_BINARY;
  #endif
  
!   if (try_cwd_first || IS_ABSOLUTE_PATH (string))
      {
        int i;
        filename = alloca (strlen (string) + 1);
--- 555,561 ----
    mode |= O_BINARY;
  #endif
  
!   if ((try_cwd_first || IS_ABSOLUTE_PATH (string)) && is_regular_file (string))
      {
        int i;
        filename = alloca (strlen (string) + 1);
*************** openp (const char *path, int try_cwd_fir
*** 601,609 ****
        strcat (filename + len, SLASH_STRING);
        strcat (filename, string);
  
!       fd = open (filename, mode);
!       if (fd >= 0)
! 	break;
      }
  
  done:
--- 613,624 ----
        strcat (filename + len, SLASH_STRING);
        strcat (filename, string);
  
!       if (is_regular_file (filename))
!       {
!         fd = open (filename, mode);
!         if (fd >= 0)
!           break;
!       }
      }
  
  done:

Re: [RFA] Should openp open directories?

[Eli Zaretskii]

On Thu, 18 Apr 2002, Joel Brobecker wrote:

> 2002-04-18  Joel Brobecker  <brobecker@gnat.com>
> 
>         * source.c (is_regular_file): New function.
>         (openp): Check wether file to open is a regular file
>         to avoid opening directories.

I think this change is a good idea, but I have one comment: if stat 
fails, isn't it better to return non-zero?  Yes, I know: it shouldn't 
happen, but if we return non-zero in that case, we keep 
back-compatibility in case there are some obscure filesystems when that 
could happen.

Also, did you try your patch when there's a directory by that name, but 
no executable program?  If so, what does the patched GDB say and/or do?

Re: [RFA] Should openp open directories?

[Joel Brobecker]

> I think this change is a good idea, but I have one comment: if stat 
> fails, isn't it better to return non-zero?  Yes, I know: it shouldn't 
> happen, but if we return non-zero in that case, we keep 
> back-compatibility in case there are some obscure filesystems when that 
> could happen.

It seems a sensible idea to return True when stat fails, except when the
file does not exist. I suggest the new following implementation (rough
code, not compiled):

   status = stat (filename, &st);

   /* A comment here explaining why we return true when errno != ENOENT.
      Or maybe in the function description ??? */
   if (status != 0)
     return (errno != ENOENT);

What do you think?

> Also, did you try your patch when there's a directory by that name, but 
> no executable program?  If so, what does the patched GDB say and/or do?

Yes. I tried "./gdb/gdb bfd" at the root of the gdb source directory and
got the following error message:

          bfd: No such file or directory.

The "or directory" part may be confusing...

-- 
Joel

Re: [RFA] Should openp open directories?

[Eli Zaretskii]

> Date: Fri, 19 Apr 2002 11:18:07 +0200
> From: Joel Brobecker <brobecker@ACT-Europe.FR>
> 
> It seems a sensible idea to return True when stat fails, except when the
> file does not exist.

This is okay with me (non-existing files will fail the open call
anyhow).

> > Also, did you try your patch when there's a directory by that name, but 
> > no executable program?  If so, what does the patched GDB say and/or do?
> 
> Yes. I tried "./gdb/gdb bfd" at the root of the gdb source directory and
> got the following error message:
> 
>           bfd: No such file or directory.

That's good.  Thanks for testing.

> The "or directory" part may be confusing...

Yes, but it's a standard string associated with ENOENT, so I guess
users of GDB should be acquainted with it.

Re: [RFA] Should openp open directories?

[Joel Brobecker]

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

> This is okay with me (non-existing files will fail the open call
> anyhow).

Sorry for the delay. Here is a new patch, based on our discussion:

2002-04-18  Joel Brobecker  <brobecker@gnat.com>

        * source.c (is_regular_file): New function.
        (openp): Check wether file to open is a regular file
        to avoid opening directories.

Ok to commit?
-- 
Joel

[-- Attachment #2: source.c.diff --]
[-- Type: text/plain, Size: 2012 bytes --]

Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.27
diff -c -3 -p -r1.27 source.c
*** source.c	12 Apr 2002 19:46:29 -0000	1.27
--- source.c	22 Apr 2002 10:59:28 -0000
*************** source_info (char *ignore, int from_tty)
*** 503,508 ****
--- 503,525 ----
  }
  \f
  
+ /* Return True if the file NAME exists and is a regular file */
+ static int
+ is_regular_file (const char *name)
+ {
+   struct stat st;
+   const int status = stat (name, &st);
+ 
+   /* Stat should never fail except when the file does not exist.
+      If stat fails, analyze the source of error and return True
+      unless the file does not exist, to avoid returning false results
+      on obscure systems where stat does not work as expected.
+    */
+   if (status != 0)
+     return (errno != ENOENT);
+ 
+   return S_ISREG (st.st_mode);
+ }
  
  /* Open a file named STRING, searching path PATH (dir names sep by some char)
     using mode MODE and protection bits PROT in the calls to open.
*************** openp (const char *path, int try_cwd_fir
*** 543,549 ****
    mode |= O_BINARY;
  #endif
  
!   if (try_cwd_first || IS_ABSOLUTE_PATH (string))
      {
        int i;
        filename = alloca (strlen (string) + 1);
--- 560,566 ----
    mode |= O_BINARY;
  #endif
  
!   if ((try_cwd_first || IS_ABSOLUTE_PATH (string)) && is_regular_file (string))
      {
        int i;
        filename = alloca (strlen (string) + 1);
*************** openp (const char *path, int try_cwd_fir
*** 601,609 ****
        strcat (filename + len, SLASH_STRING);
        strcat (filename, string);
  
!       fd = open (filename, mode);
!       if (fd >= 0)
! 	break;
      }
  
  done:
--- 618,629 ----
        strcat (filename + len, SLASH_STRING);
        strcat (filename, string);
  
!       if (is_regular_file (filename))
!       {
!         fd = open (filename, mode);
!         if (fd >= 0)
!           break;
!       }
      }
  
  done:

Re: [RFA] Should openp open directories?

[Eli Zaretskii]

> Date: Mon, 22 Apr 2002 13:00:17 +0200
> From: Joel Brobecker <brobecker@ACT-Europe.FR>
> 
> Sorry for the delay. Here is a new patch, based on our discussion:
> 
> 2002-04-18  Joel Brobecker  <brobecker@gnat.com>
> 
>         * source.c (is_regular_file): New function.
>         (openp): Check wether file to open is a regular file
>         to avoid opening directories.
> 
> Ok to commit?

Yes, thanks.

Re: [RFA] Should openp open directories?

[Joel Brobecker]

> > Ok to commit?
> 
> Yes, thanks.

Thank you. Committed to the main branch.

-- 
Joel