[PATCH] [PR symtab/17911] Recognize bad file types

[PATCH] [PR symtab/17911] Recognize bad file types

[Doug Evans]

Hi.

I tripped over this this morning, and then found the bug report.
We came up with basically the same answer, but I like EINVAL
better than ENOEXEC if given neither a file nor a directory.

Tested on amd64-linux.

2015-09-19  Doug Evans  <xdje42@gmail.com>

	PR symtab/17911
	* source.c (is_regular_file): Set errno.
	(openp): Init errno to ENOENT before iterating over search path.

	testsuite/
	* gdb.base/bad-file.exp: New file.

diff --git a/gdb/source.c b/gdb/source.c
index fab974c..34384fa 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -684,7 +684,10 @@ source_info (char *ignore, int from_tty)
 }
 \f
 
-/* Return True if the file NAME exists and is a regular file.  */
+/* Return True if the file NAME exists and is a regular file.
+   If the result is false then errno is set to a useful value assuming we're
+   expecting a regular file.  */
+
 static int
 is_regular_file (const char *name)
 {
@@ -699,7 +702,14 @@ is_regular_file (const char *name)
   if (status != 0)
     return (errno != ENOENT);
 
-  return S_ISREG (st.st_mode);
+  if (S_ISREG (st.st_mode))
+    return 1;
+
+  if (S_ISDIR (st.st_mode))
+    errno = EISDIR;
+  else
+    errno = EINVAL;
+  return 0;
 }
 
 /* Open a file named STRING, searching path PATH (dir names sep by some char)
@@ -785,6 +795,7 @@ openp (const char *path, int opts, const char *string,
 	{
 	  filename = NULL;
 	  fd = -1;
+	  /* Note: is_regular_file will have set errno appropriately.  */
 	}
 
       if (!(opts & OPF_SEARCH_IN_PATH))
@@ -808,6 +819,7 @@ openp (const char *path, int opts, const char *string,
   alloclen = strlen (path) + strlen (string) + 2;
   filename = alloca (alloclen);
   fd = -1;
+  errno = ENOENT;
 
   dir_vec = dirnames_to_char_ptr_vec (path);
   back_to = make_cleanup_free_char_ptr_vec (dir_vec);
@@ -879,6 +891,10 @@ openp (const char *path, int opts, const char *string,
 	  if (fd >= 0)
 	    break;
 	}
+      else
+	{
+	  /* Note: is_regular_file will have set errno appropriately.  */
+	}
     }
 
   do_cleanups (back_to);
diff --git a/gdb/testsuite/gdb.base/bad-file.exp b/gdb/testsuite/gdb.base/bad-file.exp
new file mode 100644
index 0000000..09da32c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bad-file.exp
@@ -0,0 +1,56 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# Test passing bad files to gdb.  PR symtab/17911
+
+# We watch for specific text for errno, so only test on systems we have
+# confidence in the error text.
+
+if { ! [ishost "*-*-linux*"] } {
+  return 0
+}
+
+# There is no such file, but we still use the normal mechanism to pick
+# its name and path.
+standard_testfile
+
+gdb_exit
+gdb_start
+
+set test "non-existent file"
+set bad_file $testfile
+remote_file host delete $bad_file
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "No such file or directory.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}
+
+set test "directory"
+set bad_file [standard_output_file {}]
+remote_exec host "mkdir -p $bad_file"
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "Is a directory.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}
+
+set test "neither file nor directory"
+set bad_file "/dev/null"
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "Invalid argument.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Pedro Alves]

On 09/19/2015 11:40 PM, Doug Evans wrote:

> diff --git a/gdb/source.c b/gdb/source.c
> index fab974c..34384fa 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -684,7 +684,10 @@ source_info (char *ignore, int from_tty)
>  }
>  \f
>  
> -/* Return True if the file NAME exists and is a regular file.  */
> +/* Return True if the file NAME exists and is a regular file.
> +   If the result is false then errno is set to a useful value assuming we're
> +   expecting a regular file.  */
> +
>  static int
>  is_regular_file (const char *name)
>  {
> @@ -699,7 +702,14 @@ is_regular_file (const char *name)
>    if (status != 0)
>      return (errno != ENOENT);
>  
> -  return S_ISREG (st.st_mode);
> +  if (S_ISREG (st.st_mode))
> +    return 1;
> +
> +  if (S_ISDIR (st.st_mode))
> +    errno = EISDIR;
> +  else
> +    errno = EINVAL;
> +  return 0;
>  }
>  
>  /* Open a file named STRING, searching path PATH (dir names sep by some char)
> @@ -785,6 +795,7 @@ openp (const char *path, int opts, const char *string,
>  	{
>  	  filename = NULL;
>  	  fd = -1;
> +	  /* Note: is_regular_file will have set errno appropriately.  */
>  	}
>  
>        if (!(opts & OPF_SEARCH_IN_PATH))
> @@ -808,6 +819,7 @@ openp (const char *path, int opts, const char *string,
>    alloclen = strlen (path) + strlen (string) + 2;
>    filename = alloca (alloclen);
>    fd = -1;
> +  errno = ENOENT;
>  
>    dir_vec = dirnames_to_char_ptr_vec (path);
>    back_to = make_cleanup_free_char_ptr_vec (dir_vec);
> @@ -879,6 +891,10 @@ openp (const char *path, int opts, const char *string,
>  	  if (fd >= 0)
>  	    break;
>  	}
> +      else
> +	{
> +	  /* Note: is_regular_file will have set errno appropriately.  */
> +	}
>      }

Seems like there are function calls after these that may
clobber errno.  I think it'd be safer to do the usual
save_errno = errno; / errno = save_errno; dance.

> +# Test passing bad files to gdb.  PR symtab/17911
> +
> +# We watch for specific text for errno, so only test on systems we have
> +# confidence in the error text.
> +
> +if { ! [ishost "*-*-linux*"] } {
> +  return 0
> +}

Same comment as to the other patch -- I think we should instead
remove this check and if the error message is different on other
hosts, then whoever cares about such hosts will adjust the
test.  I don't think there will be that many cases of
different messages.

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Doug Evans]

Pedro Alves <palves@redhat.com> writes:
> Seems like there are function calls after these that may
> clobber errno.  I think it'd be safer to do the usual
> save_errno = errno; / errno = save_errno; dance.

Does what you're saying apply to the current source base?
[I think so, just checking.
And if so, then let's fix that first.]

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Pedro Alves]

On 10/26/2015 11:44 PM, Doug Evans wrote:
> Pedro Alves <palves@redhat.com> writes:
>> Seems like there are function calls after these that may
>> clobber errno.  I think it'd be safer to do the usual
>> save_errno = errno; / errno = save_errno; dance.
> 
> Does what you're saying apply to the current source base?
> [I think so, just checking.
> And if so, then let's fix that first.]
> 

Like so.

From 0becb0a09f6a18e4edfe35bf088a0dd41e949796 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 27 Oct 2015 12:54:22 +0000
Subject: [PATCH] source.c:openp: save/restore errno

openp's return is documented as:

~~~
   If a file is found, return the descriptor.
   Otherwise, return -1, with errno set for the last name we tried to open.  */
~~~

By inspection, I noticed that there are function calls after the ones
that first set errno, and those may clobber errno.  It's safer to save
errno when see an open fail, and restore it on exit.

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2015-10-27  Pedro Alves  <palves@redhat.com>

	* source.c (openp): New local 'last_errno'.  Use it to
	save/restore errno.
---
 gdb/source.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/source.c b/gdb/source.c
index 3e5e15c..194b044 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -746,6 +746,9 @@ openp (const char *path, int opts, const char *string,
   struct cleanup *back_to;
   int ix;
   char *dir;
+  /* The errno set for the last name we tried to open (and
+     failed).  */
+  int last_errno = 0;
 
   /* The open syscall MODE parameter is not specified.  */
   gdb_assert ((mode & O_CREAT) == 0);
@@ -786,6 +789,7 @@ openp (const char *path, int opts, const char *string,
 	  filename = NULL;
 	  fd = -1;
 	}
+      last_errno = errno;
 
       if (!(opts & OPF_SEARCH_IN_PATH))
 	for (i = 0; string[i]; i++)
@@ -808,6 +812,7 @@ openp (const char *path, int opts, const char *string,
   alloclen = strlen (path) + strlen (string) + 2;
   filename = (char *) alloca (alloclen);
   fd = -1;
+  last_errno = ENOENT;
 
   dir_vec = dirnames_to_char_ptr_vec (path);
   back_to = make_cleanup_free_char_ptr_vec (dir_vec);
@@ -878,6 +883,7 @@ openp (const char *path, int opts, const char *string,
 	  fd = gdb_open_cloexec (filename, mode, 0);
 	  if (fd >= 0)
 	    break;
+	  last_errno = errno;
 	}
     }
 
@@ -895,6 +901,7 @@ done:
 	*filename_opened = gdb_abspath (filename);
     }
 
+  errno = last_errno;
   return fd;
 }
 
-- 
1.9.3

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Doug Evans]

On Tue, Oct 27, 2015 at 6:11 AM, Pedro Alves <palves@redhat.com> wrote:
> On 10/26/2015 11:44 PM, Doug Evans wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>> Seems like there are function calls after these that may
>>> clobber errno.  I think it'd be safer to do the usual
>>> save_errno = errno; / errno = save_errno; dance.
>>
>> Does what you're saying apply to the current source base?
>> [I think so, just checking.
>> And if so, then let's fix that first.]
>>
>
> Like so.

So you were referring to the existing code.
[One can see this bug easily enough, but it wasn't clear
whether you were saying my bug was introducing
another bug.]
gdb is always more broken than one imagines. :-)

LGTM.

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Pedro Alves]

On 10/27/2015 03:47 PM, Doug Evans wrote:
> On Tue, Oct 27, 2015 at 6:11 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 10/26/2015 11:44 PM, Doug Evans wrote:
>>> Pedro Alves <palves@redhat.com> writes:
>>>> Seems like there are function calls after these that may
>>>> clobber errno.  I think it'd be safer to do the usual
>>>> save_errno = errno; / errno = save_errno; dance.
>>>
>>> Does what you're saying apply to the current source base?
>>> [I think so, just checking.
>>> And if so, then let's fix that first.]
>>>
>>
>> Like so.
> 
> So you were referring to the existing code.

I wasn't ...

> [One can see this bug easily enough, but it wasn't clear
> whether you were saying my bug was introducing
> another bug.]

... because it actually first looked to me that it was your
patch that introduced the issue.  So let's say your code
made it stand out.  ;-)

> gdb is always more broken than one imagines. :-)
> 
> LGTM.

I'll push in a bit.

Thanks,
Pedro Alves

Re: [PATCH] [PR symtab/17911] Recognize bad file types

[Doug Evans]

Pedro Alves <palves@redhat.com> writes:
> On 09/19/2015 11:40 PM, Doug Evans wrote:
>
>> diff --git a/gdb/source.c b/gdb/source.c
>> index fab974c..34384fa 100644
>> --- a/gdb/source.c
>> +++ b/gdb/source.c
>> @@ -684,7 +684,10 @@ source_info (char *ignore, int from_tty)
>>  }
>>  \f
>>  
>> -/* Return True if the file NAME exists and is a regular file.  */
>> +/* Return True if the file NAME exists and is a regular file.
>> +   If the result is false then errno is set to a useful value assuming we're
>> +   expecting a regular file.  */
>> +
>>  static int
>>  is_regular_file (const char *name)
>>  {
>> @@ -699,7 +702,14 @@ is_regular_file (const char *name)
>>    if (status != 0)
>>      return (errno != ENOENT);
>>  
>> -  return S_ISREG (st.st_mode);
>> +  if (S_ISREG (st.st_mode))
>> +    return 1;
>> +
>> +  if (S_ISDIR (st.st_mode))
>> +    errno = EISDIR;
>> +  else
>> +    errno = EINVAL;
>> +  return 0;
>>  }
>>  
>>  /* Open a file named STRING, searching path PATH (dir names sep by some char)
>> @@ -785,6 +795,7 @@ openp (const char *path, int opts, const char *string,
>>  	{
>>  	  filename = NULL;
>>  	  fd = -1;
>> +	  /* Note: is_regular_file will have set errno appropriately.  */
>>  	}
>>  
>>        if (!(opts & OPF_SEARCH_IN_PATH))
>> @@ -808,6 +819,7 @@ openp (const char *path, int opts, const char *string,
>>    alloclen = strlen (path) + strlen (string) + 2;
>>    filename = alloca (alloclen);
>>    fd = -1;
>> +  errno = ENOENT;
>>  
>>    dir_vec = dirnames_to_char_ptr_vec (path);
>>    back_to = make_cleanup_free_char_ptr_vec (dir_vec);
>> @@ -879,6 +891,10 @@ openp (const char *path, int opts, const char *string,
>>  	  if (fd >= 0)
>>  	    break;
>>  	}
>> +      else
>> +	{
>> +	  /* Note: is_regular_file will have set errno appropriately.  */
>> +	}
>>      }
>
> Seems like there are function calls after these that may
> clobber errno.  I think it'd be safer to do the usual
> save_errno = errno; / errno = save_errno; dance.
>
>> +# Test passing bad files to gdb.  PR symtab/17911
>> +
>> +# We watch for specific text for errno, so only test on systems we have
>> +# confidence in the error text.
>> +
>> +if { ! [ishost "*-*-linux*"] } {
>> +  return 0
>> +}
>
> Same comment as to the other patch -- I think we should instead
> remove this check and if the error message is different on other
> hosts, then whoever cares about such hosts will adjust the
> test.  I don't think there will be that many cases of
> different messages.
>
> Otherwise LGTM.

Here is what I committed.
[This version passes errno back as an argument.]

2016-04-19  Doug Evans  <xdje42@gmail.com>

	* source.c (is_regular_file): New arg errno_ptr.
	All callers updated.

	testsuite/
	* gdb.base/bad-file.exp: New file.

diff --git a/gdb/source.c b/gdb/source.c
index a1f55b2..08cb2d3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -685,9 +685,12 @@ source_info (char *ignore, int from_tty)
 }
 \f
 
-/* Return True if the file NAME exists and is a regular file.  */
+/* Return True if the file NAME exists and is a regular file.
+   If the result is false then *ERRNO_PTR is set to a useful value assuming
+   we're expecting a regular file.  */
+
 static int
-is_regular_file (const char *name)
+is_regular_file (const char *name, int *errno_ptr)
 {
   struct stat st;
   const int status = stat (name, &st);
@@ -698,9 +701,21 @@ is_regular_file (const char *name)
      on obscure systems where stat does not work as expected.  */
 
   if (status != 0)
-    return (errno != ENOENT);
+    {
+      if (errno != ENOENT)
+	return 1;
+      *errno_ptr = ENOENT;
+      return 0;
+    }
 
-  return S_ISREG (st.st_mode);
+  if (S_ISREG (st.st_mode))
+    return 1;
+
+  if (S_ISDIR (st.st_mode))
+    *errno_ptr = EISDIR;
+  else
+    *errno_ptr = EINVAL;
+  return 0;
 }
 
 /* Open a file named STRING, searching path PATH (dir names sep by some char)
@@ -775,22 +790,23 @@ openp (const char *path, int opts, const char *string,
 
   if ((opts & OPF_TRY_CWD_FIRST) || IS_ABSOLUTE_PATH (string))
     {
-      int i;
+      int i, reg_file_errno;
 
-      if (is_regular_file (string))
+      if (is_regular_file (string, &reg_file_errno))
 	{
 	  filename = (char *) alloca (strlen (string) + 1);
 	  strcpy (filename, string);
 	  fd = gdb_open_cloexec (filename, mode, 0);
 	  if (fd >= 0)
 	    goto done;
+	  last_errno = errno;
 	}
       else
 	{
 	  filename = NULL;
 	  fd = -1;
+	  last_errno = reg_file_errno;
 	}
-      last_errno = errno;
 
       if (!(opts & OPF_SEARCH_IN_PATH))
 	for (i = 0; string[i]; i++)
@@ -821,6 +837,7 @@ openp (const char *path, int opts, const char *string,
   for (ix = 0; VEC_iterate (char_ptr, dir_vec, ix, dir); ++ix)
     {
       size_t len = strlen (dir);
+      int reg_file_errno;
 
       if (strcmp (dir, "$cwd") == 0)
 	{
@@ -879,13 +896,15 @@ openp (const char *path, int opts, const char *string,
       strcat (filename + len, SLASH_STRING);
       strcat (filename, string);
 
-      if (is_regular_file (filename))
+      if (is_regular_file (filename, &reg_file_errno))
 	{
 	  fd = gdb_open_cloexec (filename, mode, 0);
 	  if (fd >= 0)
 	    break;
 	  last_errno = errno;
 	}
+      else
+	last_errno = reg_file_errno;
     }
 
   do_cleanups (back_to);
diff --git a/gdb/testsuite/gdb.base/bad-file.exp b/gdb/testsuite/gdb.base/bad-file.exp
new file mode 100644
index 0000000..82ca3d4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bad-file.exp
@@ -0,0 +1,54 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# Test passing bad files to gdb.  PR symtab/17911
+
+# Note: While we test for specific text in error messages,
+# thus perhaps making the test host specific, if your host
+# print different text then the plan is to update the expected text
+# instead of making this test linux-only or some such.
+
+# There is no such file, but we still use the normal mechanism to pick
+# its name and path.
+standard_testfile
+
+gdb_exit
+gdb_start
+
+set test "non-existent file"
+set bad_file $testfile
+remote_file host delete $bad_file
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "No such file or directory.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}
+
+set test "directory"
+set bad_file [standard_output_file {}]
+remote_exec host "mkdir -p $bad_file"
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "Is a directory.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}
+
+set test "neither file nor directory"
+set bad_file "/dev/null"
+gdb_test_multiple "file $bad_file" "$test" {
+    -re "Invalid argument.\[\r\n\]+$gdb_prompt $" {
+	pass $test
+    }
+}