From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6581 invoked by alias); 7 Jun 2005 07:17:50 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 6495 invoked by uid 22791); 7 Jun 2005 07:17:26 -0000 Received: from admin.voldemort.codesourcery.com (HELO sethra.codesourcery.com) (65.74.133.9) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Tue, 07 Jun 2005 07:17:26 +0000 Received: from sethra.codesourcery.com (localhost.localdomain [127.0.0.1]) by sethra.codesourcery.com (8.12.11/8.12.11) with ESMTP id j577HNkA006434; Tue, 7 Jun 2005 00:17:24 -0700 Received: (from mitchell@localhost) by sethra.codesourcery.com (8.12.11/8.12.11/Submit) id j577HNq4006430; Tue, 7 Jun 2005 00:17:23 -0700 Date: Tue, 07 Jun 2005 07:17:00 -0000 Message-Id: <200506070717.j577HNq4006430@sethra.codesourcery.com> From: Mark Mitchell To: binutils@sources.redhat.com, gdb-patches@sources.redhat.com Subject: PATCH: Avoid accidentally opening files for write Reply-to: mark@codesourcery.com X-SW-Source: 2005-06/txt/msg00051.txt.bz2 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 * opncls.c (bfd_fopen): New API. (bfd_openr): Use it. (bfd_fdopenr): Likewise. * bfd-in2.h: Regenerated. 2005-06-06 Mark Mitchell * 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 <>) with the target ! @var{target}. Return a pointer to the created BFD. Calls <>, so @var{target} is interpreted as by that function. If <> is returned then an error has occured. Possible errors are <>, <> or <> 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 <> is used to open the file; otherwise, <> ! is used. @var{mode} is passed directly to <> or ! <>. Calls <>, so @var{target} is interpreted as by that function. If <> is returned then an error has occured. Possible errors are <>, <> or <> 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 <>) with the target + @var{target}. Return a pointer to the created BFD. + + Calls <>, so @var{target} is interpreted as by + that function. + + If <> is returned then an error has occured. Possible errors + are <>, <> or + <> 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,