PATCH: Avoid accidentally opening files for write

Mark Mitchell
2005-06-07T07:17:50Z

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,