diff mbox

[libfortran] Some path handling fixes

Message ID BANLkTikKTk7rrO5EVfG4aAWUGKwQyHqLVw@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist April 29, 2011, 3:20 p.m. UTC 
Hello all,

since about a decade or so ago, POSIX specifies that PATH_MAX includes
the trailing null byte (previously it was undefined). However,
libgfortran has incorrectly assumed the opposite, and has thus created
temporary buffers of length PATH_MAX + 1 for holding paths when
converting from Fortran to C style strings. Secondly, these buffers
are allocated on the stack, and on Linux PATH_MAX is 4096 (presumably
other targets use something similar), which is quite a large object to
put on the stack. This can however be easily avoided in the common
case by using VLA's or alloca() as most paths are quite short, and we
know the size of the Fortran string to convert.

The attached patch fixes the problems discussed above. Regtested on
x86_64-unknown-linux-gnu, Ok for trunk?

2011-04-29  Janne Blomqvist  <jb@gcc.gnu.org>

	* io/unix.c (min): New macro.
	(unpack_filename): Return errno number for errors.
	(regular_file): Use appropriately sized buffer for path.
	(compare_file_filename): Likewise.
	(find_file): Likewise.
	(delete_file): Likewise.
	(file_exists): Likewise.
	(file_size): Likewise.
	(inquire_sequential): Likewise.
	(inquire_direct): Likewise.
	(inquire_formatted): Likewise.
	(inquire_access): Likewise.

Comments

Steve Kargl April 29, 2011, 3:55 p.m. UTC | #1 
On Fri, Apr 29, 2011 at 06:20:17PM +0300, Janne Blomqvist wrote:
> 
> since about a decade or so ago, POSIX specifies that PATH_MAX includes
> the trailing null byte (previously it was undefined). However,
> libgfortran has incorrectly assumed the opposite, and has thus created
> temporary buffers of length PATH_MAX + 1 for holding paths when
> converting from Fortran to C style strings. Secondly, these buffers
> are allocated on the stack, and on Linux PATH_MAX is 4096 (presumably
> other targets use something similar), which is quite a large object to
> put on the stack. This can however be easily avoided in the common
> case by using VLA's or alloca() as most paths are quite short, and we
> know the size of the Fortran string to convert.
> 
> The attached patch fixes the problems discussed above. Regtested on
> x86_64-unknown-linux-gnu, Ok for trunk?
> 

OK.
Jerry DeLisle April 29, 2011, 3:57 p.m. UTC | #2 
On 04/29/2011 08:20 AM, Janne Blomqvist wrote:
> Hello all,
>
> since about a decade or so ago, POSIX specifies that PATH_MAX includes
> the trailing null byte (previously it was undefined). However,
> libgfortran has incorrectly assumed the opposite, and has thus created
> temporary buffers of length PATH_MAX + 1 for holding paths when
> converting from Fortran to C style strings. Secondly, these buffers
> are allocated on the stack, and on Linux PATH_MAX is 4096 (presumably
> other targets use something similar), which is quite a large object to
> put on the stack. This can however be easily avoided in the common
> case by using VLA's or alloca() as most paths are quite short, and we
> know the size of the Fortran string to convert.
>
> The attached patch fixes the problems discussed above. Regtested on
> x86_64-unknown-linux-gnu, Ok for trunk?

OK and thanks for the patch.

Jerry
diff mbox

Patch

diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index ee2fd17..4e4bc3b 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -41,6 +41,13 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include <errno.h>
 
 
+/* min macro that evaluates its arguments only once.  */
+#define min(a,b)		\
+  ({ typeof (a) _a = (a);	\
+    typeof (b) _b = (b);	\
+    _a < _b ? _a : _b; })
+
+
 /* For mingw, we don't identify files by their inode number, but by a
    64-bit identifier created from a BY_HANDLE_FILE_INFORMATION. */
 #ifdef __MINGW32__
@@ -996,10 +1003,10 @@  int
 unpack_filename (char *cstring, const char *fstring, int len)
 {
   if (fstring == NULL)
-    return 1;
+    return EFAULT;
   len = fstrlen (fstring, len);
   if (len >= PATH_MAX)
-    return 1;
+    return ENAMETOOLONG;
 
   memmove (cstring, fstring, len);
   cstring[len] = '\0';
@@ -1124,15 +1131,17 @@  tempfile (st_parameter_open *opp)
 static int
 regular_file (st_parameter_open *opp, unit_flags *flags)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, opp->file_len + 1)];
   int mode;
   int rwflag;
   int crflag;
   int fd;
+  int err;
 
-  if (unpack_filename (path, opp->file, opp->file_len))
+  err = unpack_filename (path, opp->file, opp->file_len);
+  if (err)
     {
-      errno = ENOENT;		/* Fake an OS error */
+      errno = err;		/* Fake an OS error */
       return -1;
     }
 
@@ -1406,7 +1415,7 @@  st_printf (const char *format, ...)
 int
 compare_file_filename (gfc_unit *u, const char *name, int len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, len + 1)];
   struct stat st;
 #ifdef HAVE_WORKING_STAT
   unix_stream *s;
@@ -1506,7 +1515,7 @@  find_file0 (gfc_unit *u, FIND_FILE0_DECL)
 gfc_unit *
 find_file (const char *file, gfc_charlen_type file_len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, file_len + 1)];
   struct stat st[1];
   gfc_unit *u;
 #if defined(__MINGW32__) && !HAVE_WORKING_STAT
@@ -1625,11 +1634,12 @@  flush_all_units (void)
 int
 delete_file (gfc_unit * u)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, u->file_len + 1)];
+  int err = unpack_filename (path, u->file, u->file_len);
 
-  if (unpack_filename (path, u->file, u->file_len))
+  if (err)
     {				/* Shouldn't be possible */
-      errno = ENOENT;
+      errno = err;
       return 1;
     }
 
@@ -1643,7 +1653,7 @@  delete_file (gfc_unit * u)
 int
 file_exists (const char *file, gfc_charlen_type file_len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, file_len + 1)];
 
   if (unpack_filename (path, file, file_len))
     return 0;
@@ -1657,7 +1667,7 @@  file_exists (const char *file, gfc_charlen_type file_len)
 GFC_IO_INT
 file_size (const char *file, gfc_charlen_type file_len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, file_len + 1)];
   struct stat statbuf;
 
   if (unpack_filename (path, file, file_len))
@@ -1678,7 +1688,7 @@  static const char yes[] = "YES", no[] = "NO", unknown[] = "UNKNOWN";
 const char *
 inquire_sequential (const char *string, int len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, len + 1)];
   struct stat statbuf;
 
   if (string == NULL ||
@@ -1702,7 +1712,7 @@  inquire_sequential (const char *string, int len)
 const char *
 inquire_direct (const char *string, int len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, len + 1)];
   struct stat statbuf;
 
   if (string == NULL ||
@@ -1726,7 +1736,7 @@  inquire_direct (const char *string, int len)
 const char *
 inquire_formatted (const char *string, int len)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, len + 1)];
   struct stat statbuf;
 
   if (string == NULL ||
@@ -1761,7 +1771,7 @@  inquire_unformatted (const char *string, int len)
 static const char *
 inquire_access (const char *string, int len, int mode)
 {
-  char path[PATH_MAX + 1];
+  char path[min(PATH_MAX, len + 1)];
 
   if (string == NULL || unpack_filename (path, string, len) ||
       access (path, mode) < 0)