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

[Doug Evans <xdje42 at gmail dot com>]

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