From 41ef41b3e0a16c9f8524870a2dc4f768c237939e Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Tue, 24 Jul 2018 16:54:41 +0200 Subject: [PATCH] hush: fix nested redirects colliding with script fds This necessitates switch from libc FILE api to a simple homegrown replacement. The change which fixes the bug here is the deleting of restore_redirected_FILEs(); line. It was prematurely moving (restoring) script fd#3. The fix is: we don't even _want_ to restore scrit fds, we are perfectly fine with them being moved. The only reason we tried to restore them is that FILE api did not allow moving of FILE->fd. function old new delta refill_HFILE_and_getc - 93 +93 hfopen - 90 +90 hfclose - 66 +66 pseudo_exec_argv 591 597 +6 hush_main 1089 1095 +6 builtin_source 209 214 +5 save_fd_on_redirect 197 200 +3 setup_redirects 320 321 +1 fgetc_interactive 235 236 +1 i_peek_and_eat_bkslash_nl 99 97 -2 expand_vars_to_list 1103 1100 -3 restore_redirects 99 52 -47 fclose_and_forget 57 - -57 remember_FILE 63 - -63 ------------------------------------------------------------------------------ (add/remove: 3/2 grow/shrink: 6/3 up/down: 271/-172) Total: 99 bytes Signed-off-by: Denys Vlasenko --- libbb/xfuncs_printf.c | 1 + shell/ash_test/ash-heredoc/heredocB.right | 3 + shell/ash_test/ash-heredoc/heredocB.tests | 12 + shell/ash_test/ash-redir/redir_script.tests | 4 + shell/hush.c | 234 ++++++++++-------- shell/hush_test/hush-heredoc/heredocB.right | 3 + shell/hush_test/hush-heredoc/heredocB.tests | 12 + shell/hush_test/hush-redir/redir_script.tests | 4 + 8 files changed, 175 insertions(+), 98 deletions(-) create mode 100644 shell/ash_test/ash-heredoc/heredocB.right create mode 100755 shell/ash_test/ash-heredoc/heredocB.tests create mode 100644 shell/hush_test/hush-heredoc/heredocB.right create mode 100755 shell/hush_test/hush-heredoc/heredocB.tests diff --git a/libbb/xfuncs_printf.c b/libbb/xfuncs_printf.c index 7247c915b..6cc60f6c0 100644 --- a/libbb/xfuncs_printf.c +++ b/libbb/xfuncs_printf.c @@ -222,6 +222,7 @@ void FAST_FUNC xdup2(int from, int to) { if (dup2(from, to) != to) bb_perror_msg_and_die("can't duplicate file descriptor"); + // " %d to %d", from, to); } // "Renumber" opened fd diff --git a/shell/ash_test/ash-heredoc/heredocB.right b/shell/ash_test/ash-heredoc/heredocB.right new file mode 100644 index 000000000..43ba0b4f9 --- /dev/null +++ b/shell/ash_test/ash-heredoc/heredocB.right @@ -0,0 +1,3 @@ +one - alpha +two - beta +three - gamma diff --git a/shell/ash_test/ash-heredoc/heredocB.tests b/shell/ash_test/ash-heredoc/heredocB.tests new file mode 100755 index 000000000..45ea4687f --- /dev/null +++ b/shell/ash_test/ash-heredoc/heredocB.tests @@ -0,0 +1,12 @@ +while read line1; do + read line2 <&3 + echo $line1 - $line2 +done <&- 3>&-" && \ test x"$fds" = x" 11>&- 3>&-" \ && { echo "Ok: script fd is not closed"; exit 0; } +# or we see that fd 3 moved to fd 10: +test x"$fds1" = x" 3>&- 4>&-" && \ +test x"$fds" = x" 10>&- 3>&-" \ +&& { echo "Ok: script fd is not closed"; exit 0; } echo "Bug: script fd is closed" echo "fds1:$fds1" diff --git a/shell/hush.c b/shell/hush.c index c26484b49..7a1513c24 100644 --- a/shell/hush.c +++ b/shell/hush.c @@ -357,6 +357,9 @@ #else # define CLEAR_RANDOM_T(rnd) ((void)0) #endif +#ifndef O_CLOEXEC +# define O_CLOEXEC 0 +#endif #ifndef F_DUPFD_CLOEXEC # define F_DUPFD_CLOEXEC F_DUPFD #endif @@ -556,11 +559,27 @@ static const char *const assignment_flag[] = { }; #endif +/* We almost can use standard FILE api, but we need an ability to move + * its fd when redirects coincide with it. No api exists for that + * (RFE for it at https://sourceware.org/bugzilla/show_bug.cgi?id=21902). + * HFILE is our internal alternative. Only supports reading. + * Since we now can, we incorporate linked list of all opened HFILEs + * into the struct (used to be a separate mini-list). + */ +typedef struct HFILE { + char *cur; + char *end; + struct HFILE *next_hfile; + int is_stdin; + int fd; + char buf[1024]; +} HFILE; + typedef struct in_str { const char *p; int peek_buf[2]; int last_char; - FILE *file; + HFILE *file; } in_str; /* The descrip member of this structure is only used to make @@ -814,14 +833,6 @@ enum { NUM_OPT_O }; - -struct FILE_list { - struct FILE_list *next; - FILE *fp; - int fd; -}; - - /* "Globals" within this file */ /* Sorted roughly by size (smaller offsets == smaller code) */ struct globals { @@ -954,7 +965,7 @@ struct globals { unsigned lineno; char *lineno_var; #endif - struct FILE_list *FILE_list; + HFILE *HFILE_list; /* Which signals have non-DFL handler (even with no traps set)? * Set at the start to: * (SIGQUIT + maybe SPECIAL_INTERACTIVE_SIGS + maybe SPECIAL_JOBSTOP_SIGS) @@ -1563,83 +1574,115 @@ static int xdup_CLOEXEC_and_close(int fd, int avoid_fd) } -/* Manipulating the list of open FILEs */ -static FILE *remember_FILE(FILE *fp) +/* Manipulating HFILEs */ +static HFILE *hfopen(const char *name) { - if (fp) { - struct FILE_list *n = xmalloc(sizeof(*n)); - n->next = G.FILE_list; - G.FILE_list = n; - n->fp = fp; - n->fd = fileno(fp); - close_on_exec_on(n->fd); + HFILE *fp; + int fd; + + fd = STDIN_FILENO; + if (name) { + fd = open(name, O_RDONLY | O_CLOEXEC); + if (fd < 0) + return NULL; + if (O_CLOEXEC == 0) /* ancient libc */ + close_on_exec_on(fd); } + + fp = xmalloc(sizeof(*fp)); + fp->is_stdin = (name == NULL); + fp->fd = fd; + fp->cur = fp->end = fp->buf; + fp->next_hfile = G.HFILE_list; + G.HFILE_list = fp; return fp; } -static void fclose_and_forget(FILE *fp) +static void hfclose(HFILE *fp) { - struct FILE_list **pp = &G.FILE_list; + HFILE **pp = &G.HFILE_list; while (*pp) { - struct FILE_list *cur = *pp; - if (cur->fp == fp) { - *pp = cur->next; - free(cur); + HFILE *cur = *pp; + if (cur == fp) { + *pp = cur->next_hfile; break; } - pp = &cur->next; + pp = &cur->next_hfile; } - fclose(fp); + if (fp->fd >= 0) + close(fp->fd); + free(fp); } -static int save_FILEs_on_redirect(int fd, int avoid_fd) +static int refill_HFILE_and_getc(HFILE *fp) { - struct FILE_list *fl = G.FILE_list; + int n; + + if (fp->fd < 0) { + /* Already saw EOF */ + return EOF; + } + /* Try to buffer more input */ + fp->cur = fp->buf; + n = safe_read(fp->fd, fp->buf, sizeof(fp->buf)); + if (n < 0) { + bb_perror_msg("read error"); + n = 0; + } + fp->end = fp->buf + n; + if (n == 0) { + /* EOF/error */ + close(fp->fd); + fp->fd = -1; + return EOF; + } + return (unsigned char)(*fp->cur++); +} +/* Inlined for common case of non-empty buffer. + */ +static ALWAYS_INLINE int hfgetc(HFILE *fp) +{ + if (fp->cur < fp->end) + return (unsigned char)(*fp->cur++); + /* Buffer empty */ + return refill_HFILE_and_getc(fp); +} +static int move_HFILEs_on_redirect(int fd, int avoid_fd) +{ + HFILE *fl = G.HFILE_list; while (fl) { if (fd == fl->fd) { /* We use it only on script files, they are all CLOEXEC */ fl->fd = xdup_CLOEXEC_and_close(fd, avoid_fd); debug_printf_redir("redirect_fd %d: matches a script fd, moving it to %d\n", fd, fl->fd); - return 1; + return 1; /* "found and moved" */ } - fl = fl->next; - } - return 0; -} -static void restore_redirected_FILEs(void) -{ - struct FILE_list *fl = G.FILE_list; - while (fl) { - int should_be = fileno(fl->fp); - if (fl->fd != should_be) { - debug_printf_redir("restoring script fd from %d to %d\n", fl->fd, should_be); - xmove_fd(fl->fd, should_be); - fl->fd = should_be; - } - fl = fl->next; + fl = fl->next_hfile; } + return 0; /* "not in the list" */ } #if ENABLE_FEATURE_SH_STANDALONE && BB_MMU -static void close_all_FILE_list(void) +static void close_all_HFILE_list(void) { - struct FILE_list *fl = G.FILE_list; + HFILE *fl = G.HFILE_list; while (fl) { - /* fclose would also free FILE object. + /* hfclose would also free HFILE object. * It is disastrous if we share memory with a vforked parent. * I'm not sure we never come here after vfork. * Therefore just close fd, nothing more. */ - /*fclose(fl->fp); - unsafe */ - close(fl->fd); - fl = fl->next; + /*hfclose(fl); - unsafe */ + if (fl->fd >= 0) + close(fl->fd); + fl = fl->next_hfile; } } #endif -static int fd_in_FILEs(int fd) +static int fd_in_HFILEs(int fd) { - struct FILE_list *fl = G.FILE_list; + HFILE *fl = G.HFILE_list; while (fl) { if (fl->fd == fd) return 1; - fl = fl->next; + fl = fl->next_hfile; } return 0; } @@ -2580,7 +2623,7 @@ static int get_user_input(struct in_str *i) } fflush_all(); //FIXME: here ^C or SIGINT will have effect only after - r = fgetc(i->file); + r = hfgetc(i->file); /* In !ENABLE_FEATURE_EDITING we don't use read_line_input, * no ^C masking happens during fgetc, no special code for ^C: * it generates SIGINT as usual. @@ -2600,22 +2643,22 @@ static int fgetc_interactive(struct in_str *i) { int ch; /* If it's interactive stdin, get new line. */ - if (G_interactive_fd && i->file == stdin) { + if (G_interactive_fd && i->file->is_stdin) { /* Returns first char (or EOF), the rest is in i->p[] */ ch = get_user_input(i); G.promptmode = 1; /* PS2 */ debug_printf_prompt("%s promptmode=%d\n", __func__, G.promptmode); } else { /* Not stdin: script file, sourced file, etc */ - do ch = fgetc(i->file); while (ch == '\0'); + do ch = hfgetc(i->file); while (ch == '\0'); } return ch; } #else -static inline int fgetc_interactive(struct in_str *i) +static ALWAYS_INLINE int fgetc_interactive(struct in_str *i) { int ch; - do ch = fgetc(i->file); while (ch == '\0'); + do ch = hfgetc(i->file); while (ch == '\0'); return ch; } #endif /* INTERACTIVE */ @@ -2730,7 +2773,7 @@ static int i_peek2(struct in_str *i) ch = i->peek_buf[1]; if (ch == 0) { /* We did not read it yet, get it now */ - do ch = fgetc(i->file); while (ch == '\0'); + do ch = hfgetc(i->file); while (ch == '\0'); i->peek_buf[1] = ch; } @@ -2774,10 +2817,10 @@ static int i_peek_and_eat_bkslash_nl(struct in_str *input) } } -static void setup_file_in_str(struct in_str *i, FILE *f) +static void setup_file_in_str(struct in_str *i, HFILE *fp) { memset(i, 0, sizeof(*i)); - i->file = f; + i->file = fp; /* i->p = NULL; */ } @@ -7106,19 +7149,19 @@ static void parse_and_run_string(const char *s) //IF_HUSH_LINENO_VAR(G.lineno = sv;) } -static void parse_and_run_file(FILE *f) +static void parse_and_run_file(HFILE *fp) { struct in_str input; IF_HUSH_LINENO_VAR(unsigned sv = G.lineno;) IF_HUSH_LINENO_VAR(G.lineno = 1;) - setup_file_in_str(&input, f); + setup_file_in_str(&input, fp); parse_and_run_stream(&input, ';'); IF_HUSH_LINENO_VAR(G.lineno = sv;) } #if ENABLE_HUSH_TICK -static FILE *generate_stream_from_string(const char *s, pid_t *pid_p) +static int generate_stream_from_string(const char *s, pid_t *pid_p) { pid_t pid; int channel[2]; @@ -7220,7 +7263,7 @@ static FILE *generate_stream_from_string(const char *s, pid_t *pid_p) free(to_free); # endif close(channel[1]); - return remember_FILE(xfdopen_for_read(channel[0])); + return channel[0]; } /* Return code is exit status of the process that is run. */ @@ -7230,7 +7273,7 @@ static int process_command_subs(o_string *dest, const char *s) pid_t pid; int status, ch, eol_cnt; - fp = generate_stream_from_string(s, &pid); + fp = xfdopen_for_read(generate_stream_from_string(s, &pid)); /* Now send results of command back into original context */ eol_cnt = 0; @@ -7249,7 +7292,7 @@ static int process_command_subs(o_string *dest, const char *s) } debug_printf("done reading from `cmd` pipe, closing it\n"); - fclose_and_forget(fp); + fclose(fp); /* We need to extract exitcode. Test case * "true; echo `sleep 1; false` $?" * should print 1 */ @@ -7427,20 +7470,17 @@ static int save_fd_on_redirect(int fd, int avoid_fd, struct squirrel **sqp) return 1; /* "we closed fd" */ } #endif + /* If this one of script's fds? */ + if (move_HFILEs_on_redirect(fd, avoid_fd)) + return 1; /* yes. "we closed fd" (actually moved it) */ + /* Are we called from setup_redirects(squirrel==NULL)? Two cases: - * (1) Redirect in a forked child. No need to save FILEs' fds, - * we aren't going to use them anymore, ok to trash. - * (2) "exec 3>FILE". Bummer. We can save script FILEs' fds, - * but how are we doing to restore them? - * "fileno(fd) = new_fd" can't be done. + * (1) Redirect in a forked child. + * (2) "exec 3>FILE". */ if (!sqp) return 0; - /* If this one of script's fds? */ - if (save_FILEs_on_redirect(fd, avoid_fd)) - return 1; /* yes. "we closed fd" */ - /* Check whether it collides with any open fds (e.g. stdio), save fds as needed */ *sqp = add_squirrel(*sqp, fd, avoid_fd); return 0; /* "we did not close fd" */ @@ -7465,8 +7505,6 @@ static void restore_redirects(struct squirrel *sq) } /* If moved, G.interactive_fd stays on new fd, not restoring it */ - - restore_redirected_FILEs(); } #if ENABLE_FEATURE_SH_STANDALONE && BB_MMU @@ -7474,7 +7512,7 @@ static void close_saved_fds_and_FILE_fds(void) { if (G_interactive_fd) close(G_interactive_fd); - close_all_FILE_list(); + close_all_HFILE_list(); } #endif @@ -7487,7 +7525,7 @@ static int internally_opened_fd(int fd, struct squirrel *sq) return 1; #endif /* If this one of script's fds? */ - if (fd_in_FILEs(fd)) + if (fd_in_HFILEs(fd)) return 1; if (sq) for (i = 0; sq[i].orig_fd >= 0; i++) { @@ -7512,7 +7550,7 @@ static int setup_redirects(struct command *prog, struct squirrel **sqp) save_fd_on_redirect(redir->rd_fd, /*avoid:*/ 0, sqp); /* for REDIRECT_HEREDOC2, rd_filename holds _contents_ * of the heredoc */ - debug_printf_parse("set heredoc '%s'\n", + debug_printf_redir("set heredoc '%s'\n", redir->rd_filename); setup_heredoc(redir); continue; @@ -7524,8 +7562,7 @@ static int setup_redirects(struct command *prog, struct squirrel **sqp) int mode; if (redir->rd_filename == NULL) { - /* - * Examples: + /* Examples: * "cmd >" (no filename) * "cmd > " (which is never interactive (unless -i?)) * sources $BASH_ENV here (without scanning $PATH). @@ -9902,13 +9938,15 @@ int hush_main(int argc, char **argv) G.global_argv++; debug_printf("running script '%s'\n", G.global_argv[0]); xfunc_error_retval = 127; /* for "hush /does/not/exist" case */ - input = xfopen_for_read(G.global_argv[0]); + input = hfopen(G.global_argv[0]); + if (!input) { + bb_simple_perror_msg_and_die(G.global_argv[0]); + } xfunc_error_retval = 1; - remember_FILE(input); install_special_sighandlers(); parse_and_run_file(input); #if ENABLE_FEATURE_CLEAN_UP - fclose_and_forget(input); + hfclose(input); #endif goto final_return; } @@ -10038,7 +10076,7 @@ int hush_main(int argc, char **argv) ); } - parse_and_run_file(stdin); + parse_and_run_file(hfopen(NULL)); /* stdin */ final_return: hush_exit(G.last_exitcode); @@ -10849,7 +10887,7 @@ Test that VAR is a valid variable name? static int FAST_FUNC builtin_source(char **argv) { char *arg_path, *filename; - FILE *input; + HFILE *input; save_arg_t sv; char *args_need_save; #if ENABLE_HUSH_FUNCTIONS @@ -10873,10 +10911,10 @@ static int FAST_FUNC builtin_source(char **argv) return EXIT_FAILURE; } } - input = remember_FILE(fopen_or_warn(filename, "r")); + input = hfopen(filename); free(arg_path); if (!input) { - /* bb_perror_msg("%s", *argv); - done by fopen_or_warn */ + bb_perror_msg("%s", filename); /* POSIX: non-interactive shell should abort here, * not merely fail. So far no one complained :) */ @@ -10895,7 +10933,7 @@ static int FAST_FUNC builtin_source(char **argv) /* "false; . ./empty_line; echo Zero:$?" should print 0 */ G.last_exitcode = 0; parse_and_run_file(input); - fclose_and_forget(input); + hfclose(input); if (args_need_save) /* can't use argv[1] instead: "shift" can mangle it */ restore_G_args(&sv, argv); diff --git a/shell/hush_test/hush-heredoc/heredocB.right b/shell/hush_test/hush-heredoc/heredocB.right new file mode 100644 index 000000000..43ba0b4f9 --- /dev/null +++ b/shell/hush_test/hush-heredoc/heredocB.right @@ -0,0 +1,3 @@ +one - alpha +two - beta +three - gamma diff --git a/shell/hush_test/hush-heredoc/heredocB.tests b/shell/hush_test/hush-heredoc/heredocB.tests new file mode 100755 index 000000000..45ea4687f --- /dev/null +++ b/shell/hush_test/hush-heredoc/heredocB.tests @@ -0,0 +1,12 @@ +while read line1; do + read line2 <&3 + echo $line1 - $line2 +done <&- 3>&-" && \ test x"$fds" = x" 11>&- 3>&-" \ && { echo "Ok: script fd is not closed"; exit 0; } +# or we see that fd 3 moved to fd 10: +test x"$fds1" = x" 3>&- 4>&-" && \ +test x"$fds" = x" 10>&- 3>&-" \ +&& { echo "Ok: script fd is not closed"; exit 0; } echo "Bug: script fd is closed" echo "fds1:$fds1"