PATCH: Avoid accidentally opening files for write

PATCH: Avoid accidentally opening files for write

[Mark Mitchell]

GDB (and probably other BFD-based tools) use the idiom:

  fd = open (pathname, flags, mode);
  bfd = bfd_fdopenr (pathname, target, fd);

Unfortunately, bfd_fdopenr has at two problems in non-POSIX
environments.  

First, and most importantly, bfd_fdopenr does: 

#if ! defined(HAVE_FCNTL) || ! defined(F_GETFL)
  fdflags = O_RDWR;			/* Assume full access.  */
#else
  fdflags = fcntl (fd, F_GETFL, NULL);
#endif

followed by:

  switch (fdflags & (O_ACCMODE))
    {
    case O_RDONLY: nbfd->iostream = fdopen (fd, FOPEN_RB);   break;
    case O_WRONLY: nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
    case O_RDWR:   nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
    default: abort ();
    }

So, without fcntl, we assume that the file should be opened for
writing.  That means that bfd_close will try to update the file with
any changes to its contents.  GDB used to have a bug that could cause
accidental corruption of the file in that case.  Also, if the file is
actually read-only, we get weird assertion failures later.  In any
case, it's just plain dangerous to open files for writing when, in
fact, the client application never asked for that.  However, there are
cases when the client is expecting a writable BFD, so it's not save
just to change the !HAVE_FCNTL case to use O_RDONLY.

The second, less important, problem is that despite the use of "O_RDWR"
for the !HAVE_FCNTL case, the !HAVE_FDOPEN code is:

  #ifndef HAVE_FDOPEN
    nbfd->iostream = fopen (filename, FOPEN_RB);
  #else

which is of course inconsistent; if we're assuming that we should use
O_RDWR above, then this should be FOPEN_RUB.
  
The attached patch introduces a new API called bfd_fopen.  This
function takes an explicit mode parameter (ala fopen) and always uses
it.  That avoids the various inconistencies above.  To reduce
duplication, bfd_fdopenr and bfd_openr are both implemented in terms
of this new API.

I've changed most uses of bfd_fdopenr in GDB to use the new API.  (I
left uses in procfs.c and rs6000-nat.c, which only get used on POSIX
systems, and where it is somewhat less convenient to use the new API.)

Tested on x86_64-unknown-linux-gnu.  OK to apply?

--
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com

2005-06-06  Mark Mitchell  <mark@codesourcery.com>

	* opncls.c (bfd_fopen): New API.
	(bfd_openr): Use it.
	(bfd_fdopenr): Likewise.
	* bfd-in2.h: Regenerated.

2005-06-06  Mark Mitchell  <mark@codesourcery.com>

	* corelow.c (core_open): Use bfd_fopen, not bfd_fdopenr.
	* exec.c (exec_file_attach): Likewise.
	* solib-frv.c (enable_break2): Likewise.
	* solib-svr4.c (enable_break): Likewise.
	* solib.c (solib_map_sections): Likewise.
	* symfile.c (symfile_bfd_open): Likewise.

Index: bfd/bfd-in2.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in2.h,v
retrieving revision 1.343
diff -c -5 -p -r1.343 bfd-in2.h
*** bfd/bfd-in2.h	6 Jun 2005 14:28:30 -0000	1.343
--- bfd/bfd-in2.h	7 Jun 2005 07:14:10 -0000
*************** extern struct coff_comdat_info *bfd_coff
*** 891,900 ****
--- 891,903 ----
  
  /* Extracted from init.c.  */
  void bfd_init (void);
  
  /* Extracted from opncls.c.  */
+ bfd *bfd_fopen (const char *filename, const char *target,
+     const char *mode, int fd);
+ 
  bfd *bfd_openr (const char *filename, const char *target);
  
  bfd *bfd_fdopenr (const char *filename, const char *target, int fd);
  
  bfd *bfd_openstreamr (const char *, const char *, void *);
Index: bfd/opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.33
diff -c -5 -p -r1.33 opncls.c
*** bfd/opncls.c	4 May 2005 15:53:36 -0000	1.33
--- bfd/opncls.c	7 Jun 2005 07:14:10 -0000
*************** SECTION
*** 126,183 ****
  
  */
  
  /*
  FUNCTION
! 	bfd_openr
  
  SYNOPSIS
! 	bfd *bfd_openr (const char *filename, const char *target);
  
  DESCRIPTION
! 	Open the file @var{filename} (using <<fopen>>) with the target
! 	@var{target}.  Return a pointer to the created BFD.
  
  	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
  	that function.
  
  	If <<NULL>> is returned then an error has occured.   Possible errors
  	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
  	<<system_call>> error.
  */
  
  bfd *
! bfd_openr (const char *filename, const char *target)
  {
    bfd *nbfd;
    const bfd_target *target_vec;
  
    nbfd = _bfd_new_bfd ();
    if (nbfd == NULL)
      return NULL;
  
    target_vec = bfd_find_target (target, nbfd);
    if (target_vec == NULL)
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
  
    nbfd->filename = filename;
-   nbfd->direction = read_direction;
  
!   if (bfd_open_file (nbfd) == NULL)
      {
-       /* File didn't exist, or some such.  */
-       bfd_set_error (bfd_error_system_call);
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
  
    return nbfd;
  }
  
  /* Don't try to `optimize' this function:
  
     o - We lock using stack space so that interrupting the locking
         won't cause a storage leak.
     o - We open the file stream last, since we don't want to have to
--- 126,232 ----
  
  */
  
  /*
  FUNCTION
! 	bfd_fopen
  
  SYNOPSIS
! 	bfd *bfd_fopen (const char *filename, const char *target,
!                         const char *mode, int fd);
  
  DESCRIPTION
! 	Open the file @var{filename} with the target @var{target}.
! 	Return a pointer to the created BFD.  If @var{fd} is not -1,
! 	then <<fdopen>> is used to open the file; otherwise, <<fopen>>
! 	is used.  @var{mode} is passed directly to <<fopen>> or
! 	<<fdopen>>. 
  
  	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
  	that function.
  
  	If <<NULL>> is returned then an error has occured.   Possible errors
  	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
  	<<system_call>> error.
  */
  
  bfd *
! bfd_fopen (const char *filename, const char *target, const char *mode, int fd)
  {
    bfd *nbfd;
    const bfd_target *target_vec;
  
+   bfd_set_error (bfd_error_system_call);
+ 
    nbfd = _bfd_new_bfd ();
    if (nbfd == NULL)
      return NULL;
  
    target_vec = bfd_find_target (target, nbfd);
    if (target_vec == NULL)
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
+   
+ #ifdef HAVE_FDOPEN
+   if (fd != -1)
+     nbfd->iostream = fdopen (fd, mode);
+   else
+ #endif
+     nbfd->iostream = fopen (filename, mode);
+   if (nbfd->iostream == NULL)
+     {
+       _bfd_delete_bfd (nbfd);
+       return NULL;
+     }
  
+   /* OK, put everything where it belongs.  */
    nbfd->filename = filename;
  
!   if (strchr (mode, '+'))
!     nbfd->direction = both_direction;
!   else if (strchr (mode, 'r'))
!     nbfd->direction = read_direction;
!   else
!     nbfd->direction = write_direction;
! 
!   if (! bfd_cache_init (nbfd))
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
+   nbfd->opened_once = TRUE;
  
    return nbfd;
  }
  
+ /*
+ FUNCTION
+ 	bfd_openr
+ 
+ SYNOPSIS
+ 	bfd *bfd_openr (const char *filename, const char *target);
+ 
+ DESCRIPTION
+ 	Open the file @var{filename} (using <<fopen>>) with the target
+ 	@var{target}.  Return a pointer to the created BFD.
+ 
+ 	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
+ 	that function.
+ 
+ 	If <<NULL>> is returned then an error has occured.   Possible errors
+ 	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
+ 	<<system_call>> error.
+ */
+ 
+ bfd *
+ bfd_openr (const char *filename, const char *target)
+ {
+   return bfd_fopen (filename, target, FOPEN_RB, -1);
+ }
+ 
  /* Don't try to `optimize' this function:
  
     o - We lock using stack space so that interrupting the locking
         won't cause a storage leak.
     o - We open the file stream last, since we don't want to have to
*************** DESCRIPTION
*** 210,285 ****
  */
  
  bfd *
  bfd_fdopenr (const char *filename, const char *target, int fd)
  {
!   bfd *nbfd;
!   const bfd_target *target_vec;
!   int fdflags;
  
    bfd_set_error (bfd_error_system_call);
  #if ! defined(HAVE_FCNTL) || ! defined(F_GETFL)
!   fdflags = O_RDWR;			/* Assume full access.  */
  #else
    fdflags = fcntl (fd, F_GETFL, NULL);
- #endif
    if (fdflags == -1)
      return NULL;
  
-   nbfd = _bfd_new_bfd ();
-   if (nbfd == NULL)
-     return NULL;
- 
-   target_vec = bfd_find_target (target, nbfd);
-   if (target_vec == NULL)
-     {
-       _bfd_delete_bfd (nbfd);
-       return NULL;
-     }
- 
- #ifndef HAVE_FDOPEN
-   nbfd->iostream = fopen (filename, FOPEN_RB);
- #else
    /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
    switch (fdflags & (O_ACCMODE))
      {
!     case O_RDONLY: nbfd->iostream = fdopen (fd, FOPEN_RB);   break;
!     case O_WRONLY: nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
!     case O_RDWR:   nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
      default: abort ();
      }
  #endif
  
!   if (nbfd->iostream == NULL)
!     {
!       _bfd_delete_bfd (nbfd);
!       return NULL;
!     }
! 
!   /* OK, put everything where it belongs.  */
!   nbfd->filename = filename;
! 
!   /* As a special case we allow a FD open for read/write to
!      be written through, although doing so requires that we end
!      the previous clause with a preposition.  */
!   /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
!   switch (fdflags & (O_ACCMODE))
!     {
!     case O_RDONLY: nbfd->direction = read_direction; break;
!     case O_WRONLY: nbfd->direction = write_direction; break;
!     case O_RDWR: nbfd->direction = both_direction; break;
!     default: abort ();
!     }
! 
!   if (! bfd_cache_init (nbfd))
!     {
!       _bfd_delete_bfd (nbfd);
!       return NULL;
!     }
!   nbfd->opened_once = TRUE;
! 
!   return nbfd;
  }
  
  /*
  FUNCTION
  	bfd_openstreamr
--- 259,290 ----
  */
  
  bfd *
  bfd_fdopenr (const char *filename, const char *target, int fd)
  {
!   const char *mode;
  
    bfd_set_error (bfd_error_system_call);
  #if ! defined(HAVE_FCNTL) || ! defined(F_GETFL)
!   mode = FOPEN_RUB; /* Assume full access.  */
  #else
+   int fdflags;
    fdflags = fcntl (fd, F_GETFL, NULL);
    if (fdflags == -1)
      return NULL;
  
    /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
    switch (fdflags & (O_ACCMODE))
      {
!     case O_RDONLY: mode = FOPEN_RB;
!     case O_WRONLY: mode = FOPEN_RUB;
!     case O_RDWR:   mode = FOPEN_RUB;
      default: abort ();
      }
  #endif
  
!   return bfd_fopen (filename, target, mode, fd);
  }
  
  /*
  FUNCTION
  	bfd_openstreamr
Index: gdb/corelow.c
===================================================================
RCS file: /cvs/src/src/gdb/corelow.c,v
retrieving revision 1.50
diff -c -5 -p -r1.50 corelow.c
*** gdb/corelow.c	16 May 2005 16:36:23 -0000	1.50
--- gdb/corelow.c	7 Jun 2005 07:14:10 -0000
*************** core_open (char *filename, int from_tty)
*** 314,324 ****
      flags |= O_RDONLY;
    scratch_chan = open (filename, flags, 0);
    if (scratch_chan < 0)
      perror_with_name (filename);
  
!   temp_bfd = bfd_fdopenr (filename, gnutarget, scratch_chan);
    if (temp_bfd == NULL)
      perror_with_name (filename);
  
    if (!bfd_check_format (temp_bfd, bfd_core) &&
        !gdb_check_format (temp_bfd))
--- 314,326 ----
      flags |= O_RDONLY;
    scratch_chan = open (filename, flags, 0);
    if (scratch_chan < 0)
      perror_with_name (filename);
  
!   temp_bfd = bfd_fopen (filename, gnutarget, 
! 			write_files ? FOPEN_RUB : FOPEN_RB,
! 			scratch_chan);
    if (temp_bfd == NULL)
      perror_with_name (filename);
  
    if (!bfd_check_format (temp_bfd, bfd_core) &&
        !gdb_check_format (temp_bfd))
Index: gdb/exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.55
diff -c -5 -p -r1.55 exec.c
*** gdb/exec.c	16 May 2005 04:45:43 -0000	1.55
--- gdb/exec.c	7 Jun 2005 07:14:10 -0000
*************** exec_file_attach (char *filename, int fr
*** 212,222 ****
  	     &scratch_pathname);
  	}
  #endif
        if (scratch_chan < 0)
  	perror_with_name (filename);
!       exec_bfd = bfd_fdopenr (scratch_pathname, gnutarget, scratch_chan);
  
        if (!exec_bfd)
  	error (_("\"%s\": could not open as an executable file: %s"),
  	       scratch_pathname, bfd_errmsg (bfd_get_error ()));
  
--- 212,224 ----
  	     &scratch_pathname);
  	}
  #endif
        if (scratch_chan < 0)
  	perror_with_name (filename);
!       exec_bfd = bfd_fopen (scratch_pathname, gnutarget,
! 			    write_files ? FOPEN_RUB : FOPEN_RB,
! 			    scratch_chan);
  
        if (!exec_bfd)
  	error (_("\"%s\": could not open as an executable file: %s"),
  	       scratch_pathname, bfd_errmsg (bfd_get_error ()));
  
Index: gdb/solib-frv.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-frv.c,v
retrieving revision 1.9
diff -c -5 -p -r1.9 solib-frv.c
*** gdb/solib-frv.c	29 Apr 2005 21:48:28 -0000	1.9
--- gdb/solib-frv.c	7 Jun 2005 07:14:10 -0000
*************** enable_break2 (void)
*** 646,656 ****
           be trivial on GNU/Linux).  Therefore, we have to try an alternate
           mechanism to find the dynamic linker's base address.  */
  
        tmp_fd  = solib_open (buf, &tmp_pathname);
        if (tmp_fd >= 0)
! 	tmp_bfd = bfd_fdopenr (tmp_pathname, gnutarget, tmp_fd);
  
        if (tmp_bfd == NULL)
  	{
  	  enable_break_failure_warning ();
  	  return 0;
--- 646,656 ----
           be trivial on GNU/Linux).  Therefore, we have to try an alternate
           mechanism to find the dynamic linker's base address.  */
  
        tmp_fd  = solib_open (buf, &tmp_pathname);
        if (tmp_fd >= 0)
! 	tmp_bfd = bfd_fopen (tmp_pathname, gnutarget, FOPEN_RB, tmp_fd);
  
        if (tmp_bfd == NULL)
  	{
  	  enable_break_failure_warning ();
  	  return 0;
Index: gdb/solib-svr4.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.c,v
retrieving revision 1.50
diff -c -5 -p -r1.50 solib-svr4.c
*** gdb/solib-svr4.c	6 Jun 2005 22:24:25 -0000	1.50
--- gdb/solib-svr4.c	7 Jun 2005 07:14:10 -0000
*************** enable_break (void)
*** 881,891 ****
           be trivial on GNU/Linux).  Therefore, we have to try an alternate
           mechanism to find the dynamic linker's base address.  */
  
        tmp_fd  = solib_open (buf, &tmp_pathname);
        if (tmp_fd >= 0)
! 	tmp_bfd = bfd_fdopenr (tmp_pathname, gnutarget, tmp_fd);
  
        if (tmp_bfd == NULL)
  	goto bkpt_at_symbol;
  
        /* Make sure the dynamic linker's really a useful object.  */
--- 881,891 ----
           be trivial on GNU/Linux).  Therefore, we have to try an alternate
           mechanism to find the dynamic linker's base address.  */
  
        tmp_fd  = solib_open (buf, &tmp_pathname);
        if (tmp_fd >= 0)
! 	tmp_bfd = bfd_fopen (tmp_pathname, gnutarget, FOPEN_RB, tmp_fd);
  
        if (tmp_bfd == NULL)
  	goto bkpt_at_symbol;
  
        /* Make sure the dynamic linker's really a useful object.  */
Index: gdb/solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.80
diff -c -5 -p -r1.80 solib.c
*** gdb/solib.c	30 Apr 2005 12:59:57 -0000	1.80
--- gdb/solib.c	7 Jun 2005 07:14:10 -0000
*************** solib_map_sections (void *arg)
*** 275,285 ****
      {
        perror_with_name (filename);
      }
  
    /* Leave scratch_pathname allocated.  abfd->name will point to it.  */
!   abfd = bfd_fdopenr (scratch_pathname, gnutarget, scratch_chan);
    if (!abfd)
      {
        close (scratch_chan);
        error (_("Could not open `%s' as an executable file: %s"),
  	     scratch_pathname, bfd_errmsg (bfd_get_error ()));
--- 275,285 ----
      {
        perror_with_name (filename);
      }
  
    /* Leave scratch_pathname allocated.  abfd->name will point to it.  */
!   abfd = bfd_fopen (scratch_pathname, gnutarget, FOPEN_RB, scratch_chan);
    if (!abfd)
      {
        close (scratch_chan);
        error (_("Could not open `%s' as an executable file: %s"),
  	     scratch_pathname, bfd_errmsg (bfd_get_error ()));
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.158
diff -c -5 -p -r1.158 symfile.c
*** gdb/symfile.c	8 May 2005 14:46:52 -0000	1.158
--- gdb/symfile.c	7 Jun 2005 07:14:10 -0000
*************** symfile_bfd_open (char *name)
*** 1278,1288 ****
      }
    xfree (name);			/* Free 1st new malloc'd copy */
    name = absolute_name;		/* Keep 2nd malloc'd copy in bfd */
    /* It'll be freed in free_objfile(). */
  
!   sym_bfd = bfd_fdopenr (name, gnutarget, desc);
    if (!sym_bfd)
      {
        close (desc);
        make_cleanup (xfree, name);
        error (_("\"%s\": can't open to read symbols: %s."), name,
--- 1278,1288 ----
      }
    xfree (name);			/* Free 1st new malloc'd copy */
    name = absolute_name;		/* Keep 2nd malloc'd copy in bfd */
    /* It'll be freed in free_objfile(). */
  
!   sym_bfd = bfd_fopen (name, gnutarget, FOPEN_RB, desc);
    if (!sym_bfd)
      {
        close (desc);
        make_cleanup (xfree, name);
        error (_("\"%s\": can't open to read symbols: %s."), name,

Re: PATCH: Avoid accidentally opening files for write

[Nick Clifton]

Hi Mark,

> Tested on x86_64-unknown-linux-gnu.  OK to apply?

Have you tested this patch in both POSIX and non-POSIX build 
environments ?  Given your comments with the original posting I would 
assume so, but it would be nice to have that confirmed.  Also with a 
patch to a generic part of BFD it would be good if you could also test 
with a --enable-targets=all build, just to make sure.

> 2005-06-06  Mark Mitchell  <mark@codesourcery.com>
> 
> 	* opncls.c (bfd_fopen): New API.
> 	(bfd_openr): Use it.
> 	(bfd_fdopenr): Likewise.
> 	* bfd-in2.h: Regenerated.

The bfd part of your patch is approved.

I do have one other concern however:

>   SYNOPSIS
> ! 	bfd *bfd_fopen (const char *filename, const char *target,
> !                         const char *mode, int fd);
>   
>   DESCRIPTION
> ! 	Open the file @var{filename} with the target @var{target}.
> ! 	Return a pointer to the created BFD.  If @var{fd} is not -1,
> ! 	then <<fdopen>> is used to open the file; otherwise, <<fopen>>
> ! 	is used.  @var{mode} is passed directly to <<fopen>> or
> ! 	<<fdopen>>. 
...
> !   if (strchr (mode, '+'))
> !     nbfd->direction = both_direction;
> !   else if (strchr (mode, 'r'))
> !     nbfd->direction = read_direction;
> !   else
> !     nbfd->direction = write_direction;

This assumes that the contents of 'mode' are well defined.  Is this the 
case for non-POSIX environments ?  For example can we be sure that the 
character 'R' is never used to indicate read-only status, or that an OS 
might allow a file created with just "a" to have the newly-written-to 
parts read back, effectively making "a" a read-and-write mode ?  What I 
am getting at is, should bfd_fopen() take an explicit extra parameter 
which tells BFD whether this file is intended for reading, writing or both ?

Cheers
   Nick

Re: PATCH: Avoid accidentally opening files for write

[Mark Mitchell]

Nick Clifton wrote:
> Hi Mark,
> 
>> Tested on x86_64-unknown-linux-gnu.  OK to apply?
> 
> 
> Have you tested this patch in both POSIX and non-POSIX build 
> environments ?  

I did not test in a non-POSIX *build* environment, but I did test in a 
non-POIX *target* environment.  In particular, I built a MinGW GDB using 
a GNU/Linux host, and verified that this fixed the bug.  (I used an 
older version of GDB that still had a tendency to mangle its output; 
using this patch stopped it from doing the mangling because it no longer 
tried to write out the file.)

 > assume so, but it would be nice to have that confirmed.  Also with a
> patch to a generic part of BFD it would be good if you could also test 
> with a --enable-targets=all build, just to make sure.

I didn't know about that, but will try it before check-in.  Just a 
build, or should I try to run some kind of testsuite as well?  (Is there 
a way to run the binutils testuite on all targets all at once?)

>> !   if (strchr (mode, '+'))
>> !     nbfd->direction = both_direction;
>> !   else if (strchr (mode, 'r'))
>> !     nbfd->direction = read_direction;
>> !   else
>> !     nbfd->direction = write_direction;
> 
> 
> This assumes that the contents of 'mode' are well defined.  Is this the 
> case for non-POSIX environments ?  For example can we be sure that the 
> character 'R' is never used to indicate read-only status, or that an OS 
> might allow a file created with just "a" to have the newly-written-to 
> parts read back, effectively making "a" a read-and-write mode ?  What I 
> am getting at is, should bfd_fopen() take an explicit extra parameter 
> which tells BFD whether this file is intended for reading, writing or 
> both ?

Hmm.  In practice, we always use one of the FOPEN_* macros as an 
argument, and these do follow the rules implied by what I wrote.  But, I 
could tighten the test to check for just what ISO C requires, which is 
that the characters must occur at the start of the string, so using 
strchr is probably incorrect.  OK to make that change before check-in, 
or would you like me to resubmit?

I'm not aware of OSes that do as you say, but, in any case, I don't 
think we need to worry about OSes that accept other variations.  Clients 
of BFD should be using the standard syntax.  It's OK if they use OS 
extensions, but I think it's reasonable to say that if they mean "read" 
they use "r" and not "R".

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304

Re: PATCH: Avoid accidentally opening files for write

[Nick Clifton]

Hi Mark,

>> patch to a generic part of BFD it would be good if you could also test 
>> with a --enable-targets=all build, just to make sure.

> I didn't know about that, but will try it before check-in. 

Thanks.

> Just a build, or should I try to run some kind of testsuite as well?

Just a build, since there will no binutils executable which will 
exercise the new code anyway.  (You could run a GDB testsuite with a 
patched GDB linked against the BFD library built for an all targets 
toolchain but I think that is needlessly paranoid).

 > (Is there a way to run the binutils testuite on all targets all at once?)

Not really.  You can run the binutils testsuites with an all-targets 
toolchain but it will just check the toolchain's default target, not all 
of the targets it can possibly support.

>> This assumes that the contents of 'mode' are well defined.  Is this 
>> the case for non-POSIX environments ?  For example can we be sure that 
>> the character 'R' is never used to indicate read-only status, or that 
>> an OS might allow a file created with just "a" to have the 
>> newly-written-to parts read back, effectively making "a" a 
>> read-and-write mode ?  What I am getting at is, should bfd_fopen() 
>> take an explicit extra parameter which tells BFD whether this file is 
>> intended for reading, writing or both ?
> 
> 
> Hmm.  In practice, we always use one of the FOPEN_* macros as an 
> argument, and these do follow the rules implied by what I wrote.  But, I 
> could tighten the test to check for just what ISO C requires, which is 
> that the characters must occur at the start of the string, so using 
> strchr is probably incorrect.  OK to make that change before check-in, 
> or would you like me to resubmit?

No please just make the change before check-in.

> I'm not aware of OSes that do as you say, but, in any case, I don't 
> think we need to worry about OSes that accept other variations.  Clients 
> of BFD should be using the standard syntax.  It's OK if they use OS 
> extensions, but I think it's reasonable to say that if they mean "read" 
> they use "r" and not "R".

Fair enough.

Cheers
   Nick

Re: PATCH: Avoid accidentally opening files for write

[Mark Mitchell]

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

Nick Clifton wrote:
> Hi Mark,
> 
>>> patch to a generic part of BFD it would be good if you could also 
>>> test with a --enable-targets=all build, just to make sure.
> 
> 
>> I didn't know about that, but will try it before check-in. 

It worked -- except for a warning from the new ms1 port when compiling 
readelf.  I worked around that, and committed anyhow, as that was an 
unrelated issue.

>> I could tighten the test to check for just what ISO C requires, which 
>> is that the characters must occur at the start of the string, so using 
>> strchr is probably incorrect.
> 
> No please just make the change before check-in.

I've attached the version committed.  It's now on both mainline and 
csl-arm-20050325-branch.  Thanks for the quick review!

GDB folks, may I now commit the other half of the patch?

Thanks,

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304

[-- Attachment #2: bfd.fopen.patch --]
[-- Type: text/plain, Size: 8262 bytes --]

2005-06-06  Mark Mitchell  <mark@codesourcery.com>

	* opncls.c (bfd_fopen): New API.
	(bfd_openr): Use it.
	(bfd_fdopenr): Likewise.
	* bfd-in2.h: Regenerated.

Index: bfd-in2.h
===================================================================
RCS file: /cvs/src/src/bfd/bfd-in2.h,v
retrieving revision 1.344
diff -c -5 -p -r1.344 bfd-in2.h
*** bfd-in2.h	7 Jun 2005 21:07:29 -0000	1.344
--- bfd-in2.h	7 Jun 2005 22:52:03 -0000
*************** extern struct coff_comdat_info *bfd_coff
*** 891,900 ****
--- 891,903 ----
  
  /* Extracted from init.c.  */
  void bfd_init (void);
  
  /* Extracted from opncls.c.  */
+ bfd *bfd_fopen (const char *filename, const char *target,
+     const char *mode, int fd);
+ 
  bfd *bfd_openr (const char *filename, const char *target);
  
  bfd *bfd_fdopenr (const char *filename, const char *target, int fd);
  
  bfd *bfd_openstreamr (const char *, const char *, void *);
Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.33
diff -c -5 -p -r1.33 opncls.c
*** opncls.c	4 May 2005 15:53:36 -0000	1.33
--- opncls.c	7 Jun 2005 22:52:03 -0000
*************** SECTION
*** 126,183 ****
  
  */
  
  /*
  FUNCTION
! 	bfd_openr
  
  SYNOPSIS
! 	bfd *bfd_openr (const char *filename, const char *target);
  
  DESCRIPTION
! 	Open the file @var{filename} (using <<fopen>>) with the target
! 	@var{target}.  Return a pointer to the created BFD.
  
  	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
  	that function.
  
  	If <<NULL>> is returned then an error has occured.   Possible errors
  	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
  	<<system_call>> error.
  */
  
  bfd *
! bfd_openr (const char *filename, const char *target)
  {
    bfd *nbfd;
    const bfd_target *target_vec;
  
    nbfd = _bfd_new_bfd ();
    if (nbfd == NULL)
      return NULL;
  
    target_vec = bfd_find_target (target, nbfd);
    if (target_vec == NULL)
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
  
    nbfd->filename = filename;
-   nbfd->direction = read_direction;
  
!   if (bfd_open_file (nbfd) == NULL)
      {
-       /* File didn't exist, or some such.  */
-       bfd_set_error (bfd_error_system_call);
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
  
    return nbfd;
  }
  
  /* Don't try to `optimize' this function:
  
     o - We lock using stack space so that interrupting the locking
         won't cause a storage leak.
     o - We open the file stream last, since we don't want to have to
--- 126,235 ----
  
  */
  
  /*
  FUNCTION
! 	bfd_fopen
  
  SYNOPSIS
! 	bfd *bfd_fopen (const char *filename, const char *target,
!                         const char *mode, int fd);
  
  DESCRIPTION
! 	Open the file @var{filename} with the target @var{target}.
! 	Return a pointer to the created BFD.  If @var{fd} is not -1,
! 	then <<fdopen>> is used to open the file; otherwise, <<fopen>>
! 	is used.  @var{mode} is passed directly to <<fopen>> or
! 	<<fdopen>>. 
  
  	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
  	that function.
  
  	If <<NULL>> is returned then an error has occured.   Possible errors
  	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
  	<<system_call>> error.
  */
  
  bfd *
! bfd_fopen (const char *filename, const char *target, const char *mode, int fd)
  {
    bfd *nbfd;
    const bfd_target *target_vec;
  
+   bfd_set_error (bfd_error_system_call);
+ 
    nbfd = _bfd_new_bfd ();
    if (nbfd == NULL)
      return NULL;
  
    target_vec = bfd_find_target (target, nbfd);
    if (target_vec == NULL)
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
+   
+ #ifdef HAVE_FDOPEN
+   if (fd != -1)
+     nbfd->iostream = fdopen (fd, mode);
+   else
+ #endif
+     nbfd->iostream = fopen (filename, mode);
+   if (nbfd->iostream == NULL)
+     {
+       _bfd_delete_bfd (nbfd);
+       return NULL;
+     }
  
+   /* OK, put everything where it belongs.  */
    nbfd->filename = filename;
  
!   /* Figure out whether the user is opening the file for reading,
!      writing, or both, by looking at the MODE argument.  */
!   if ((mode[0] == 'r' || mode[0] == 'w' || mode[0] == 'a') 
!       && mode[1] == '+')
!     nbfd->direction = both_direction;
!   else if (mode[0] == 'r')
!     nbfd->direction = read_direction;
!   else
!     nbfd->direction = write_direction;
! 
!   if (! bfd_cache_init (nbfd))
      {
        _bfd_delete_bfd (nbfd);
        return NULL;
      }
+   nbfd->opened_once = TRUE;
  
    return nbfd;
  }
  
+ /*
+ FUNCTION
+ 	bfd_openr
+ 
+ SYNOPSIS
+ 	bfd *bfd_openr (const char *filename, const char *target);
+ 
+ DESCRIPTION
+ 	Open the file @var{filename} (using <<fopen>>) with the target
+ 	@var{target}.  Return a pointer to the created BFD.
+ 
+ 	Calls <<bfd_find_target>>, so @var{target} is interpreted as by
+ 	that function.
+ 
+ 	If <<NULL>> is returned then an error has occured.   Possible errors
+ 	are <<bfd_error_no_memory>>, <<bfd_error_invalid_target>> or
+ 	<<system_call>> error.
+ */
+ 
+ bfd *
+ bfd_openr (const char *filename, const char *target)
+ {
+   return bfd_fopen (filename, target, FOPEN_RB, -1);
+ }
+ 
  /* Don't try to `optimize' this function:
  
     o - We lock using stack space so that interrupting the locking
         won't cause a storage leak.
     o - We open the file stream last, since we don't want to have to
*************** DESCRIPTION
*** 210,285 ****
  */
  
  bfd *
  bfd_fdopenr (const char *filename, const char *target, int fd)
  {
!   bfd *nbfd;
!   const bfd_target *target_vec;
    int fdflags;
  
    bfd_set_error (bfd_error_system_call);
  #if ! defined(HAVE_FCNTL) || ! defined(F_GETFL)
!   fdflags = O_RDWR;			/* Assume full access.  */
  #else
    fdflags = fcntl (fd, F_GETFL, NULL);
- #endif
    if (fdflags == -1)
      return NULL;
  
-   nbfd = _bfd_new_bfd ();
-   if (nbfd == NULL)
-     return NULL;
- 
-   target_vec = bfd_find_target (target, nbfd);
-   if (target_vec == NULL)
-     {
-       _bfd_delete_bfd (nbfd);
-       return NULL;
-     }
- 
- #ifndef HAVE_FDOPEN
-   nbfd->iostream = fopen (filename, FOPEN_RB);
- #else
    /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
    switch (fdflags & (O_ACCMODE))
      {
!     case O_RDONLY: nbfd->iostream = fdopen (fd, FOPEN_RB);   break;
!     case O_WRONLY: nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
!     case O_RDWR:   nbfd->iostream = fdopen (fd, FOPEN_RUB);  break;
      default: abort ();
      }
  #endif
  
!   if (nbfd->iostream == NULL)
!     {
!       _bfd_delete_bfd (nbfd);
!       return NULL;
!     }
! 
!   /* OK, put everything where it belongs.  */
!   nbfd->filename = filename;
! 
!   /* As a special case we allow a FD open for read/write to
!      be written through, although doing so requires that we end
!      the previous clause with a preposition.  */
!   /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
!   switch (fdflags & (O_ACCMODE))
!     {
!     case O_RDONLY: nbfd->direction = read_direction; break;
!     case O_WRONLY: nbfd->direction = write_direction; break;
!     case O_RDWR: nbfd->direction = both_direction; break;
!     default: abort ();
!     }
! 
!   if (! bfd_cache_init (nbfd))
!     {
!       _bfd_delete_bfd (nbfd);
!       return NULL;
!     }
!   nbfd->opened_once = TRUE;
! 
!   return nbfd;
  }
  
  /*
  FUNCTION
  	bfd_openstreamr
--- 262,295 ----
  */
  
  bfd *
  bfd_fdopenr (const char *filename, const char *target, int fd)
  {
!   const char *mode;
! #if defined(HAVE_FCNTL) && defined(F_GETFL)
    int fdflags;
+ #endif
  
    bfd_set_error (bfd_error_system_call);
  #if ! defined(HAVE_FCNTL) || ! defined(F_GETFL)
!   mode = FOPEN_RUB; /* Assume full access.  */
  #else
    fdflags = fcntl (fd, F_GETFL, NULL);
    if (fdflags == -1)
      return NULL;
  
    /* (O_ACCMODE) parens are to avoid Ultrix header file bug.  */
    switch (fdflags & (O_ACCMODE))
      {
!     case O_RDONLY: mode = FOPEN_RB;
!     case O_WRONLY: mode = FOPEN_RUB;
!     case O_RDWR:   mode = FOPEN_RUB;
      default: abort ();
      }
  #endif
  
!   return bfd_fopen (filename, target, mode, fd);
  }
  
  /*
  FUNCTION
  	bfd_openstreamr

Re: PATCH: Avoid accidentally opening files for write

[Daniel Jacobowitz]

On Tue, Jun 07, 2005 at 12:17:23AM -0700, Mark Mitchell wrote:
> 2005-06-06  Mark Mitchell  <mark@codesourcery.com>
> 
> 	* corelow.c (core_open): Use bfd_fopen, not bfd_fdopenr.
> 	* exec.c (exec_file_attach): Likewise.
> 	* solib-frv.c (enable_break2): Likewise.
> 	* solib-svr4.c (enable_break): Likewise.
> 	* solib.c (solib_map_sections): Likewise.
> 	* symfile.c (symfile_bfd_open): Likewise.

The GDB portions of this patch are OK, now that the bfd side has been
hammered out.  Thanks.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PATCH: Avoid accidentally opening files for write

[Mark Mitchell]

Daniel Jacobowitz wrote:
> On Tue, Jun 07, 2005 at 12:17:23AM -0700, Mark Mitchell wrote:
> 
>>2005-06-06  Mark Mitchell  <mark@codesourcery.com>
>>
>>	* corelow.c (core_open): Use bfd_fopen, not bfd_fdopenr.
>>	* exec.c (exec_file_attach): Likewise.
>>	* solib-frv.c (enable_break2): Likewise.
>>	* solib-svr4.c (enable_break): Likewise.
>>	* solib.c (solib_map_sections): Likewise.
>>	* symfile.c (symfile_bfd_open): Likewise.
> 
> 
> The GDB portions of this patch are OK, now that the bfd side has been
> hammered out.  Thanks.

Now applied to mainline and csl-arm-20050325-branch.

Thanks,

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304