diff options
| -rw-r--r-- | ChangeLog | 16 | ||||
| -rw-r--r-- | NEWS | 11 | ||||
| -rw-r--r-- | src/c99-to-c89.diff | 14 | ||||
| -rw-r--r-- | src/remove.c | 136 |
4 files changed, 114 insertions, 63 deletions
@@ -1,3 +1,19 @@ +2007-10-08 Jim Meyering <meyering@redhat.com> + + rm could malfunction under unusual circumstances: + When operating on a relative name longer than 511 bytes, + and (when either processing a directory that is neither writable + nor readable (but still searchable) or when determining whether + to prompt), and encountering an ENOMEM error while forming the + file name, rm would operate on a truncated-to-511-byte name + starting with "[...]" rather than the intended one. + * NEWS: Describe the bugs. + * src/remove.c: Correct two misuses of full_filename: + (full_filename0, xfull_filename): New functions. + (full_filename_): Rewrite to use full_filename0. + (AD_pop_and_chdir): Use xfull_filename, not full_filename. + (write_protected_non_symlink): Likewise. + 2007-10-07 Jim Meyering <meyering@redhat.com> Don't let a helper function modify errno. @@ -199,6 +199,17 @@ GNU coreutils NEWS -*- outline -*- pwd and "readlink -e ." no longer fail unnecessarily when a parent directory is unreadable. + rm (without -f) could prompt when it shouldn't, or fail to prompt + when it should, when operating on a full name longer than 511 bytes + and getting an ENOMEM error while trying to form the long name. + + rm could mistakenly traverse into the wrong directory under unusual + conditions: when a full name longer than 511 bytes specifies a search-only + directory, and when forming that name fails with ENOMEM, rm would attempt + to open a truncated-to-511-byte name with the first five bytes replaced + with "[...]". If such a directory were to actually exist, rm would attempt + to remove it. + "rm -rf /etc/passwd" (run by non-root) now prints a diagnostic. Before it would print nothing. diff --git a/src/c99-to-c89.diff b/src/c99-to-c89.diff index 8a3737247..f6c7664ed 100644 --- a/src/c99-to-c89.diff +++ b/src/c99-to-c89.diff @@ -1,7 +1,7 @@ diff -upr src/remove.c src/remove.c --- src/remove.c 2007-07-23 12:56:20.000000000 +0200 +++ src/remove.c 2007-07-23 13:03:12.000000000 +0200 -@@ -257,9 +257,10 @@ pop_dir (Dirstack_state *ds) +@@ -262,9 +262,10 @@ pop_dir (Dirstack_state *ds) { size_t n_lengths = obstack_object_size (&ds->len_stack) / sizeof (size_t); size_t *length = obstack_base (&ds->len_stack); @@ -13,7 +13,7 @@ diff -upr src/remove.c src/remove.c assert (top_len >= 2); /* Pop the specified length of file name. */ -@@ -391,10 +392,11 @@ AD_stack_top (Dirstack_state const *ds) +@@ -419,10 +420,11 @@ AD_stack_top (Dirstack_state const *ds) static void AD_stack_pop (Dirstack_state *ds) { @@ -26,7 +26,7 @@ diff -upr src/remove.c src/remove.c if (top->unremovable) hash_free (top->unremovable); obstack_blank (&ds->Active_dir, -(int) sizeof (struct AD_ent)); -@@ -876,6 +878,7 @@ prompt (int fd_cwd, Dirstack_state const +@@ -904,6 +906,7 @@ prompt (int fd_cwd, Dirstack_state const break; } @@ -34,7 +34,7 @@ diff -upr src/remove.c src/remove.c char const *quoted_name = quote (full_filename (filename)); if (0 < write_protected) -@@ -915,6 +918,7 @@ prompt (int fd_cwd, Dirstack_state const +@@ -943,6 +946,7 @@ prompt (int fd_cwd, Dirstack_state const : _("%s: remove %s %s? ")), program_name, file_type (sbuf), quoted_name); } @@ -42,7 +42,7 @@ diff -upr src/remove.c src/remove.c if (!yesno ()) return RM_USER_DECLINED; -@@ -1534,6 +1538,7 @@ rm_1 (Dirstack_state *ds, char const *fi +@@ -1562,6 +1566,7 @@ rm_1 (Dirstack_state *ds, char const *fi return RM_ERROR; } @@ -50,7 +50,7 @@ diff -upr src/remove.c src/remove.c struct stat st; cache_stat_init (&st); cycle_check_init (&ds->cycle_check_state); -@@ -1556,6 +1561,7 @@ rm_1 (Dirstack_state *ds, char const *fi +@@ -1584,6 +1589,7 @@ rm_1 (Dirstack_state *ds, char const *fi AD_push_initial (ds); AD_INIT_OTHER_MEMBERS (); @@ -58,7 +58,7 @@ diff -upr src/remove.c src/remove.c enum RM_status status = remove_entry (AT_FDCWD, ds, filename, DT_UNKNOWN, &st, x); if (status == RM_NONEMPTY_DIR) -@@ -1574,6 +1580,8 @@ rm_1 (Dirstack_state *ds, char const *fi +@@ -1602,6 +1608,8 @@ rm_1 (Dirstack_state *ds, char const *fi ds_clear (ds); return status; } diff --git a/src/remove.c b/src/remove.c index 4f91dd420..1023048ff 100644 --- a/src/remove.c +++ b/src/remove.c @@ -165,6 +165,11 @@ struct dirstack_state }; typedef struct dirstack_state Dirstack_state; +/* A static buffer and its allocated size, these variables are used by + xfull_filename and full_filename to form full, relative file names. */ +static char *g_buf; +static size_t g_n_allocated; + /* Like fstatat, but cache the result. If ST->st_size is -1, the status has not been gotten yet. If less than -1, fstatat failed with errno == -1 - ST->st_size. Otherwise, the status has already @@ -302,83 +307,102 @@ right_justify (char *dst, size_t dst_len, const char *src, size_t src_len, return dst_len - src_len; } -/* Using the global directory name obstack, create the full name FILENAME. +/* Using the global directory name obstack, create the full name of FILENAME. Return it in sometimes-realloc'd space that should not be freed by the - caller. Realloc as necessary. If realloc fails, use a static buffer - and put as long a suffix in that buffer as possible. Be careful not - to change errno. */ + caller. Realloc as necessary. If realloc fails, return NULL. */ -#define full_filename(Filename) full_filename_ (ds, Filename) static char * -full_filename_ (Dirstack_state const *ds, const char *filename) +full_filename0 (Dirstack_state const *ds, const char *filename) { - static char *buf = NULL; - static size_t n_allocated = 0; - int saved_errno = errno; - size_t dir_len = obstack_object_size (&ds->dir_stack); char *dir_name = obstack_base (&ds->dir_stack); - size_t n_bytes_needed; - size_t filename_len; + size_t filename_len = strlen (filename); + size_t n_bytes_needed = dir_len + filename_len + 1; - filename_len = strlen (filename); - n_bytes_needed = dir_len + filename_len + 1; - - if (n_allocated < n_bytes_needed) + if (g_n_allocated < n_bytes_needed) { - /* This code requires that realloc accept NULL as the first arg. - This function must not use xrealloc. Otherwise, an out-of-memory - error involving a file name to be expanded here wouldn't ever - be issued. Use realloc and fall back on using a static buffer - if memory allocation fails. */ - char *new_buf = realloc (buf, n_bytes_needed); - n_allocated = n_bytes_needed; - + char *new_buf = realloc (g_buf, n_bytes_needed); if (new_buf == NULL) - { -#define SBUF_SIZE 512 -#define ELLIPSES_PREFIX "[...]" - static char static_buf[SBUF_SIZE]; - bool truncated; - size_t len; - char *p; - - free (buf); - len = right_justify (static_buf, SBUF_SIZE, filename, - filename_len + 1, &p, &truncated); - right_justify (static_buf, len, dir_name, dir_len, &p, &truncated); - if (truncated) - { - memcpy (static_buf, ELLIPSES_PREFIX, - sizeof (ELLIPSES_PREFIX) - 1); - } - errno = saved_errno; - return p; - } + return NULL; - buf = new_buf; + g_buf = new_buf; + g_n_allocated = n_bytes_needed; } - if (filename_len == 1 && *filename == '.' && dir_len) + if (STREQ (filename, ".") && dir_len) { /* FILENAME is just `.' and dir_len is nonzero. Copy the directory part, omitting the trailing slash, and append a trailing zero byte. */ - char *p = mempcpy (buf, dir_name, dir_len - 1); + char *p = mempcpy (g_buf, dir_name, dir_len - 1); *p = 0; } else { /* Copy the directory part, including trailing slash, and then append the filename part, including a trailing zero byte. */ - memcpy (mempcpy (buf, dir_name, dir_len), filename, filename_len + 1); - assert (strlen (buf) + 1 == n_bytes_needed); + memcpy (mempcpy (g_buf, dir_name, dir_len), filename, filename_len + 1); + assert (strlen (g_buf) + 1 == n_bytes_needed); } - errno = saved_errno; + return g_buf; +} + +/* Using the global directory name obstack, create the full name of FILENAME. + Return it in sometimes-realloc'd space that should not be freed by the + caller. Realloc as necessary. If realloc fails, die. */ + +static char * +xfull_filename (Dirstack_state const *ds, const char *filename) +{ + char *buf = full_filename0 (ds, filename); + if (buf == NULL) + xalloc_die (); return buf; } +/* Using the global directory name obstack, create the full name FILENAME. + Return it in sometimes-realloc'd space that should not be freed by the + caller. Realloc as necessary. If realloc fails, use a static buffer + and put as long a suffix in that buffer as possible. Be careful not + to change errno. */ + +#define full_filename(Filename) full_filename_ (ds, Filename) +static char * +full_filename_ (Dirstack_state const *ds, const char *filename) +{ + int saved_errno = errno; + char *full_name = full_filename0 (ds, filename); + if (full_name) + { + errno = saved_errno; + return full_name; + } + + { +#define SBUF_SIZE 512 +#define ELLIPSES_PREFIX "[...]" + static char static_buf[SBUF_SIZE]; + bool truncated; + size_t len; + char *p; + char *dir_name = obstack_base (&ds->dir_stack); + size_t dir_len = obstack_object_size (&ds->dir_stack); + + free (g_buf); + len = right_justify (static_buf, SBUF_SIZE, filename, + strlen (filename) + 1, &p, &truncated); + right_justify (static_buf, len, dir_name, dir_len, &p, &truncated); + if (truncated) + { + memcpy (static_buf, ELLIPSES_PREFIX, + sizeof (ELLIPSES_PREFIX) - 1); + } + errno = saved_errno; + return p; + } +} + static inline size_t AD_stack_height (Dirstack_state const *ds) { @@ -512,7 +536,7 @@ AD_pop_and_chdir (DIR *dirp, int *fdp, Dirstack_state *ds) searchable, when using Solaris' openat. Without this openat call, tests/rm2 would fail to remove directories a/2 and a/3. */ if (fd < 0) - fd = openat (AT_FDCWD, full_filename ("."), O_RDONLY); + fd = openat (AT_FDCWD, xfull_filename (ds, "."), O_RDONLY); if (fd < 0) { @@ -765,13 +789,13 @@ write_protected_non_symlink (int fd_cwd, Disadvantage: changes working directory (not reentrant) and can't work if save_cwd fails. - 3) if (euidaccess (full_filename (file), W_OK) == 0) - Disadvantage: doesn't work if full_filename is too long. + 3) if (euidaccess (xfull_filename (file), W_OK) == 0) + Disadvantage: doesn't work if xfull_filename is too long. Inefficient for very deep trees (O(Depth^2)). 4) If the full pathname is sufficiently short (say, less than PATH_MAX or 8192 bytes, whichever is shorter): - use method (3) (i.e., euidaccess (full_filename (file), W_OK)); + use method (3) (i.e., euidaccess (xfull_filename (file), W_OK)); Otherwise: vfork, fchdir in the child, run euidaccess in the child, then the child exits with a status that tells the parent whether euidaccess succeeded. @@ -785,7 +809,7 @@ write_protected_non_symlink (int fd_cwd, 5) If the full file name is sufficiently short (say, less than PATH_MAX or 8192 bytes, whichever is shorter): - use method (3) (i.e., euidaccess (full_filename (file), W_OK)); + use method (3) (i.e., euidaccess (xfull_filename (file), W_OK)); Otherwise: look just at the file bits. Perhaps issue a warning the first time this occurs. @@ -801,7 +825,7 @@ write_protected_non_symlink (int fd_cwd, if (MIN (PATH_MAX, 8192) <= file_name_len) return - euidaccess_stat (buf, W_OK); - if (euidaccess (full_filename (file), W_OK) == 0) + if (euidaccess (xfull_filename (ds, file), W_OK) == 0) return 0; if (errno == EACCES) return -1; |