diff mbox

[PR,symtab/17911] Recognize bad file types

Message ID m34miqm3rq.fsf@seba.sebabeach.org
State New
Headers show

Commit Message

Doug Evans Sept. 19, 2015, 10:40 p.m. UTC
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.

Comments

Pedro Alves Sept. 29, 2015, 2:29 p.m. UTC | #1
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)
>  }
>  
>  
> -/* 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
Doug Evans Oct. 26, 2015, 11:44 p.m. UTC | #2
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.]
diff mbox

Patch

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)
 }
 
 
-/* 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
+    }
+}