From: Denis Vlasenko Date: Sun, 15 Mar 2009 15:54:58 +0000 (-0000) Subject: ftpd: fix the bug where >2GB file ops report errors; X-Git-Tag: 1_14_0~240 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=fc58ba1298519712bf42e44ab71a916fb0dfba05;p=oweals%2Fbusybox.git ftpd: fix the bug where >2GB file ops report errors; make a few simplifications; add TODOs. function old new delta port_or_pasv_was_seen - 37 +37 get_remote_transfer_fd 104 109 +5 handle_upload_common 265 260 -5 handle_dir_common 228 223 -5 popen_ls 211 203 -8 ftpd_main 1825 1815 -10 data_transfer_checks_ok 37 - -37 ------------------------------------------------------------------------------ (add/remove: 1/1 grow/shrink: 1/4 up/down: 42/-65) Total: -23 bytes --- diff --git a/networking/ftpd.c b/networking/ftpd.c index 3faa3ed7d..37b340e78 100644 --- a/networking/ftpd.c +++ b/networking/ftpd.c @@ -268,6 +268,29 @@ handle_stat(void) STR(FTP_STATOK)" Ok\r\n"); } +/* TODO: implement FEAT. Example: +# nc -vvv ftp.kernel.org 21 +ftp.kernel.org (130.239.17.4:21) open +220 Welcome to ftp.kernel.org. +FEAT +211-Features: + EPRT + EPSV + MDTM + PASV + REST STREAM + SIZE + TVFS + UTF8 +211 End +HELP +214-The following commands are recognized. + ABOR ACCT ALLO APPE CDUP CWD DELE EPRT EPSV FEAT HELP LIST MDTM MKD + MODE NLST NOOP OPTS PASS PASV PORT PWD QUIT REIN REST RETR RMD RNFR + RNTO SITE SIZE SMNT STAT STOR STOU STRU SYST TYPE USER XCUP XCWD XMKD + XPWD XRMD +214 Help OK. +*/ static void handle_help(void) { @@ -283,6 +306,29 @@ handle_help(void) /* Download commands */ +static inline int +port_active(void) +{ + return (G.port_addr != NULL); +} + +static inline int +pasv_active(void) +{ + return (G.pasv_listen_fd > STDOUT_FILENO); +} + +static void +port_pasv_cleanup(void) +{ + free(G.port_addr); + G.port_addr = NULL; + if (G.pasv_listen_fd > STDOUT_FILENO) + close(G.pasv_listen_fd); + G.pasv_listen_fd = -1; +} + +/* On error, emits error code to the peer */ static int ftpdataio_get_pasv_fd(void) { @@ -299,28 +345,26 @@ ftpdataio_get_pasv_fd(void) return remote_fd; } -static inline int -port_active(void) -{ - return (G.port_addr != NULL); -} - -static inline int -pasv_active(void) -{ - return (G.pasv_listen_fd > STDOUT_FILENO); -} - +/* Clears port/pasv data. + * This means we dont waste resources, for example, keeping + * PASV listening socket open when it is no longer needed. + * On error, emits error code to the peer (or exits). + * On success, emits p_status_msg to the peer. + */ static int get_remote_transfer_fd(const char *p_status_msg) { int remote_fd; if (pasv_active()) + /* On error, emits error code to the peer */ remote_fd = ftpdataio_get_pasv_fd(); else + /* Exits on error */ remote_fd = xconnect_stream(G.port_addr); + port_pasv_cleanup(); + if (remote_fd < 0) return remote_fd; @@ -328,8 +372,9 @@ get_remote_transfer_fd(const char *p_status_msg) return remote_fd; } +/* If there were neither PASV nor PORT, emits error code to the peer */ static int -data_transfer_checks_ok(void) +port_or_pasv_was_seen(void) { if (!pasv_active() && !port_active()) { cmdio_write_raw(STR(FTP_BADSENDCONN)" Use PORT or PASV first\r\n"); @@ -339,16 +384,7 @@ data_transfer_checks_ok(void) return 1; } -static void -port_pasv_cleanup(void) -{ - free(G.port_addr); - G.port_addr = NULL; - if (G.pasv_listen_fd > STDOUT_FILENO) - close(G.pasv_listen_fd); - G.pasv_listen_fd = -1; -} - +/* Exits on error */ static unsigned bind_for_passive_mode(void) { @@ -370,6 +406,7 @@ bind_for_passive_mode(void) return port; } +/* Exits on error */ static void handle_pasv(void) { @@ -391,6 +428,7 @@ handle_pasv(void) free(response); } +/* Exits on error */ static void handle_epsv(void) { @@ -408,16 +446,16 @@ handle_port(void) { unsigned short port; char *raw, *port_part; - len_and_sockaddr *lsa = NULL; + len_and_sockaddr *lsa; port_pasv_cleanup(); raw = G.ftp_arg; - /* buglets: - * xatou16 will accept wrong input, - * xatou16 will exit instead of generating error to peer - */ +/* Buglets: + * xatou16 will accept wrong input, + * xatou16 will exit instead of generating error to peer + */ port_part = strrchr(raw, ','); if (port_part == NULL) @@ -440,6 +478,11 @@ handle_port(void) return; } +/* Should we verify that lsa matches getpeername(STDIN)? + * Otherwise peer can make us open data connections + * to other hosts (security problem!) + */ + G.port_addr = lsa; cmdio_write_ok(FTP_PORTOK); } @@ -456,38 +499,38 @@ static void handle_retr(void) { struct stat statbuf; - int trans_ret; + off_t bytes_transferred; int remote_fd; - int opened_file; + int local_file_fd; off_t offset = G.restart_pos; char *response; G.restart_pos = 0; - if (!data_transfer_checks_ok()) - return; /* data_transfer_checks_ok emitted error response */ + if (!port_or_pasv_was_seen()) + return; /* port_or_pasv_was_seen emitted error response */ /* O_NONBLOCK is useful if file happens to be a device node */ - opened_file = G.ftp_arg ? open(G.ftp_arg, O_RDONLY | O_NONBLOCK) : -1; - if (opened_file < 0) { + local_file_fd = G.ftp_arg ? open(G.ftp_arg, O_RDONLY | O_NONBLOCK) : -1; + if (local_file_fd < 0) { cmdio_write_error(FTP_FILEFAIL); return; } - if (fstat(opened_file, &statbuf) != 0 || !S_ISREG(statbuf.st_mode)) { + if (fstat(local_file_fd, &statbuf) != 0 || !S_ISREG(statbuf.st_mode)) { /* Note - pretend open failed */ cmdio_write_error(FTP_FILEFAIL); goto file_close_out; } - /* Now deactive O_NONBLOCK, otherwise we have a problem on DMAPI filesystems - * such as XFS DMAPI. + /* Now deactive O_NONBLOCK, otherwise we have a problem + * on DMAPI filesystems such as XFS DMAPI. */ - ndelay_off(opened_file); + ndelay_off(local_file_fd); /* Set the download offset (from REST) if any */ if (offset != 0) - xlseek(opened_file, offset, SEEK_SET); + xlseek(local_file_fd, offset, SEEK_SET); response = xasprintf( " Opening BINARY mode data connection for %s (%"OFF_FMT"u bytes)", @@ -495,20 +538,22 @@ handle_retr(void) remote_fd = get_remote_transfer_fd(response); free(response); if (remote_fd < 0) - goto port_pasv_cleanup_out; + goto file_close_out; + +/* TODO: if we'll implement timeout, this will need more clever handling. + * Perhaps alarm(N) + checking that current position on local_file_fd + * is advancing. As of now, peer may stall us indefinitely. + */ - trans_ret = bb_copyfd_eof(opened_file, remote_fd); + bytes_transferred = bb_copyfd_eof(local_file_fd, remote_fd); close(remote_fd); - if (trans_ret < 0) + if (bytes_transferred < 0) cmdio_write_error(FTP_BADSENDFILE); else cmdio_write_ok(FTP_TRANSFEROK); - port_pasv_cleanup_out: - port_pasv_cleanup(); - file_close_out: - close(opened_file); + close(local_file_fd); } /* List commands */ @@ -522,12 +567,10 @@ popen_ls(const char *opt) pid_t pid; cwd = xrealloc_getcwd_or_warn(NULL); - xpiped_pair(outfd); - fflush(NULL); + /*fflush(NULL); - so far we dont use stdio on output */ pid = vfork(); - switch (pid) { case -1: /* failure */ bb_perror_msg_and_die("vfork"); @@ -547,12 +590,18 @@ popen_ls(const char *opt) } _exit(127); } + /* parent */ close(outfd.wr); free(cwd); return outfd.rd; } +enum { + USE_CTRL_CONN = 1, + LONG_LISTING = 2, +}; + static void handle_dir_common(int opts) { @@ -560,15 +609,15 @@ handle_dir_common(int opts) char *line; int ls_fd; - if (!(opts & 1) && !data_transfer_checks_ok()) - return; /* data_transfer_checks_ok emitted error response */ + if (!(opts & USE_CTRL_CONN) && !port_or_pasv_was_seen()) + return; /* port_or_pasv_was_seen emitted error response */ - ls_fd = popen_ls((opts & 2) ? "-l" : "-1"); + ls_fd = popen_ls((opts & LONG_LISTING) ? "-l" : "-1"); ls_fp = fdopen(ls_fd, "r"); if (!ls_fp) /* never happens. paranoia */ bb_perror_msg_and_die("fdopen"); - if (opts & 1) { + if (opts & USE_CTRL_CONN) { /* STAT */ cmdio_write_raw(STR(FTP_STATFILE_OK)"-Status follows:\r\n"); while (1) { @@ -587,13 +636,16 @@ handle_dir_common(int opts) line = xmalloc_fgetline(ls_fp); if (!line) break; + /* I've seen clients complaining when they + * are fed with ls output with bare '\n'. + * Pity... that would be much simpler. + */ xwrite_str(remote_fd, line); xwrite(remote_fd, "\r\n", 2); free(line); } } close(remote_fd); - port_pasv_cleanup(); cmdio_write_ok(FTP_TRANSFEROK); } fclose(ls_fp); /* closes ls_fd too */ @@ -601,7 +653,7 @@ handle_dir_common(int opts) static void handle_list(void) { - handle_dir_common(2); + handle_dir_common(LONG_LISTING); } static void handle_nlst(void) @@ -611,7 +663,7 @@ handle_nlst(void) static void handle_stat_file(void) { - handle_dir_common(3); + handle_dir_common(LONG_LISTING + USE_CTRL_CONN); } static void @@ -698,7 +750,7 @@ static void handle_upload_common(int is_append, int is_unique) { char *tempname = NULL; - int trans_ret; + off_t bytes_transferred; int local_file_fd; int remote_fd; off_t offset; @@ -706,8 +758,8 @@ handle_upload_common(int is_append, int is_unique) offset = G.restart_pos; G.restart_pos = 0; - if (!data_transfer_checks_ok()) - return; + if (!port_or_pasv_was_seen()) + return; /* port_or_pasv_was_seen emitted error response */ local_file_fd = -1; if (is_unique) { @@ -726,7 +778,7 @@ handle_upload_common(int is_append, int is_unique) return; } - /* TODO: paranoia: fstat it, refuse to do anything if it's not a regular file */ +/* TODO: paranoia: fstat it, refuse to do anything if it's not a regular file */ if (offset) xlseek(local_file_fd, offset, SEEK_SET); @@ -737,16 +789,18 @@ handle_upload_common(int is_append, int is_unique) if (remote_fd < 0) goto bail; - trans_ret = bb_copyfd_eof(remote_fd, local_file_fd); - close(remote_fd); +/* TODO: if we'll implement timeout, this will need more clever handling. + * Perhaps alarm(N) + checking that current position on local_file_fd + * is advancing. As of now, peer may stall us indefinitely. + */ - if (trans_ret < 0) + bytes_transferred = bb_copyfd_eof(remote_fd, local_file_fd); + close(remote_fd); + if (bytes_transferred < 0) cmdio_write_error(FTP_BADSENDFILE); else cmdio_write_ok(FTP_TRANSFEROK); - bail: - port_pasv_cleanup(); close(local_file_fd); }