From: sgunderson Date: Tue, 9 Oct 2001 14:28:20 +0000 (+0000) Subject: Fixed a problem where the server might segfault if there was an error occuring while... X-Git-Url: https://git.sesse.net/?p=betaftpd;a=commitdiff_plain;h=8c87c5d1792e7dd55f8b07f717d329f612e263dc Fixed a problem where the server might segfault if there was an error occuring while sending the numeric() results (this patch is not 100% tried and stable yet, unfortunately). Thanks to Dominic Rath and the rest of the chip.de team for finding and tracking down this bug. --- diff --git a/cmds.c b/cmds.c index 0809537..3c91790 100644 --- a/cmds.c +++ b/cmds.c @@ -149,7 +149,7 @@ extern fd_set master_fds, master_send_fds; struct handler { char cmd_name[6]; char add_cmlen; /* =1 if the command takes an argument */ - int (*callback)(struct conn * const); + void (*callback)(struct conn * const); char min_auth; #if !WANT_NONROOT char do_setuid; /* =1 if root is not *really* needed */ @@ -263,7 +263,7 @@ int do_chdir(struct conn * const c, const char * const newd) * authentication work. User names are limited to 16 * characters, by force... */ -int cmd_user(struct conn * const c) +void cmd_user(struct conn * const c) { strncpy(c->username, c->recv_buf, 16); c->username[16] = 0; @@ -278,7 +278,6 @@ int cmd_user(struct conn * const c) numeric(c, 331, "Password required for %s.", c->username); c->auth = 2; } - return 1; } /* @@ -289,7 +288,7 @@ int cmd_user(struct conn * const c) * don't even support PAM or real shadow passwords (with * expiry etc) yet... */ -int cmd_pass(struct conn * const c) +void cmd_pass(struct conn * const c) { #if WANT_NONROOT c->auth = nr_userinfo(c->username, &c->uid, c->curr_dir, c->root_dir, @@ -348,7 +347,6 @@ int cmd_pass(struct conn * const c) /* Have a different message for anonymous users? */ numeric(c, 230, "User logged in."); } - return 1; } /* @@ -362,10 +360,9 @@ int cmd_pass(struct conn * const c) * I feel that the RFC959 intention is having it _before_ * USER/PASS. Therefore, this one runs with root privilegies :-) */ -int cmd_acct(struct conn * const c) +void cmd_acct(struct conn * const c) { numeric(c, 202, "ACCT ignored OK -- not applicable on this system."); - return 1; } /* @@ -375,7 +372,7 @@ int cmd_acct(struct conn * const c) * the whole way), in case there are some weird overflows * somewhere. */ -int cmd_port(struct conn * const c) +void cmd_port(struct conn * const c) { short int a0, a1, a2, a3, p0, p1; int i, sock, err; @@ -384,11 +381,11 @@ int cmd_port(struct conn * const c) if ((c->transfer != NULL) && (c->transfer->state >= 4)) { numeric(c, 500, "Sorry, only one transfer at a time."); - return 1; + return; } sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); - TRAP_ERROR(sock == -1, 500, return 1); + TRAP_ERROR(sock == -1, 500, return); destroy_ftran(c->transfer); c->transfer = f = alloc_new_ftran(sock, c); @@ -403,7 +400,7 @@ int cmd_port(struct conn * const c) /* bind to own address, port 20 (FTP data) */ tmp = sizeof(sin); err = getsockname(c->sock, (struct sockaddr *)&sin, &tmp); - TRAP_ERROR(err == -1, 500, return 1); + TRAP_ERROR(err == -1, 500, return); sin.sin_port = FTP_PORT - 1; numeric(c, 200, "PORT command OK."); @@ -435,14 +432,13 @@ int cmd_port(struct conn * const c) i = 1; ioctl(f->sock, FIONBIO, &one); } - return 1; } /* * cmd_pasv(): Handles the PASV command, and sets up the data socket. * Uses port numbers > 1024, since it doesn't run as root. */ -int cmd_pasv(struct conn * const c) +void cmd_pasv(struct conn * const c) { struct ftran *f; int tmp, sock, err; @@ -451,14 +447,14 @@ int cmd_pasv(struct conn * const c) if ((c->transfer != NULL) && (c->transfer->state >= 4)) { numeric(c, 503, "Sorry, only one transfer at once."); - return 1; + return; } destroy_ftran(c->transfer); sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); - TRAP_ERROR(sock == -1, 500, return 1); + TRAP_ERROR(sock == -1, 500, return); err = add_fd(sock, POLLIN); - TRAP_ERROR(err != 0, 501, return 1); + TRAP_ERROR(err != 0, 501, return); c->transfer = f = alloc_new_ftran(sock, c); @@ -467,18 +463,18 @@ int cmd_pasv(struct conn * const c) /* setup socket */ tmp = sizeof(addr); err = getsockname(c->sock, (struct sockaddr *)&addr, &tmp); - TRAP_ERROR(err == -1, 500, return 1); + TRAP_ERROR(err == -1, 500, return); addr.sin_port = 0; /* let the system choose */ err = bind(sock, (struct sockaddr *)&addr, sizeof(struct sockaddr)); - TRAP_ERROR(err == -1, 500, return 1); + TRAP_ERROR(err == -1, 500, return); tmp = sizeof(addr); err = getsockname(sock, (struct sockaddr *)&addr, &tmp); - TRAP_ERROR(err == -1, 500, return 1); + TRAP_ERROR(err == -1, 500, return); err = listen(f->sock, 1); - TRAP_ERROR(err == -1, 500, return 1); + TRAP_ERROR(err == -1, 500, return); f->state = 1; numeric(c, 227, "Entering passive mode (%u,%u,%u,%u,%u,%u)", @@ -488,7 +484,6 @@ int cmd_pasv(struct conn * const c) (htonl(addr.sin_addr.s_addr) & 0x000000ff), (htons(addr.sin_port) & 0xff00) >> 8, (htons(addr.sin_port) & 0x00ff)); - return 1; } /* @@ -499,7 +494,7 @@ int cmd_pasv(struct conn * const c) * /betaftpd.users file, if you use nonroot. If not, it's a bug. * Try to get it _reproducible_, and mail it to me. */ -int cmd_pwd(struct conn * const c) +void cmd_pwd(struct conn * const c) { char temp[512], *cdir = NULL; @@ -507,7 +502,6 @@ int cmd_pwd(struct conn * const c) if (cdir != NULL) { numeric(c, 257, "\"%s\" is current working directory.", cdir); } - return 1; } /* @@ -522,6 +516,7 @@ char *do_pwd(struct conn * const c, char * const retbuf, const char * const dir) char *cdir = NULL; strcpy(retbuf, dir); + if (strncmp(retbuf, c->root_dir, strlen(c->root_dir)) != 0) { numeric(c, 550, "curr_dir is outside root_dir, please contact site administrator."); return NULL; @@ -541,10 +536,9 @@ char *do_pwd(struct conn * const c, char * const retbuf, const char * const dir) * cmd_cwd(): Handles CWD command (change working directory). Uses * cmd_cwd_internal() (see below). */ -int cmd_cwd(struct conn * const c) +void cmd_cwd(struct conn * const c) { cmd_cwd_internal(c, c->recv_buf); - return 1; } /* @@ -557,10 +551,9 @@ int cmd_cwd(struct conn * const c) * an error, instead of just staying in the root directory (as * the OS and thus wu-ftpd does). */ -int cmd_cdup(struct conn * const c) +void cmd_cdup(struct conn * const c) { cmd_cwd_internal(c, ".."); - return 1; } /* @@ -595,11 +588,10 @@ void cmd_cwd_internal(struct conn * const c, const char * const newd) * sending functions to start at the correct number. We should * perhaps add some better error checking to this? */ -int cmd_rest(struct conn * const c) +void cmd_rest(struct conn * const c) { c->rest_pos = abs(atoi(c->recv_buf)); numeric(c, 350, "Setting resume at %u bytes.", c->rest_pos); - return 1; } /* @@ -609,19 +601,19 @@ int cmd_rest(struct conn * const c) * connection occurs (or succeeds, if we're using PORT mode), * the actual file transfer begins. */ -int cmd_retr(struct conn * const c) +void cmd_retr(struct conn * const c) { struct ftran *f = c->transfer; if ((f == NULL) || ((f->state != 1) && (f->state != 3))) { numeric(c, 425, "No data connection set up; please use PASV or PORT."); - return 1; + return; } #if WANT_ASCII if ((c->rest_pos > 0) && (c->ascii_mode == 1)) { numeric(c, 500, "Cannot resume while in ASCII mode."); - return 1; + return; } #endif @@ -644,7 +636,6 @@ int cmd_retr(struct conn * const c) #endif prepare_for_transfer(f); } - return 1; } #if WANT_UPLOAD @@ -652,20 +643,18 @@ int cmd_retr(struct conn * const c) * cmd_stor(): Handles the STOR command (upload file). Pushes the * work down to do_store(), below. */ -int cmd_stor(struct conn * const c) +void cmd_stor(struct conn * const c) { do_store(c, 0); - return 1; } /* * cmd_appe(): Handles the APPE command (append to file). Pushes * the work down to do_store(), below. */ -int cmd_appe(struct conn * const c) +void cmd_appe(struct conn * const c) { do_store(c, 1); - return 1; } /* @@ -722,22 +711,21 @@ void do_store(struct conn * const c, const int append) * size of all the files in the directory, rather how * much space the directory inode uses. */ -int cmd_size(struct conn * const c) +void cmd_size(struct conn * const c) { #if WANT_ASCII if (c->ascii_mode) { numeric(c, 550, "SIZE not available in ASCII mode."); - return 1; + return; } #endif { const char * const fname = translate_path(c, c->recv_buf); struct stat buf; - TRAP_ERROR(fname == NULL || lstat(fname, &buf) == -1, 550, return 1); + TRAP_ERROR(fname == NULL || lstat(fname, &buf) == -1, 550, return); numeric(c, 213, "%lu", (unsigned long)(buf.st_size)); - return 1; } } @@ -746,86 +734,81 @@ int cmd_size(struct conn * const c) * date/time of a file. See the comments on cmd_size(), * above. */ -int cmd_mdtm(struct conn * const c) +void cmd_mdtm(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); struct stat buf; struct tm *m; - TRAP_ERROR(fname == NULL || lstat(fname, &buf) == -1, 550, return 1); + TRAP_ERROR(fname == NULL || lstat(fname, &buf) == -1, 550, return); m = gmtime(&(buf.st_mtime)); /* at least wu-ftpd does it in GMT */ numeric(c, 213, "%u%02u%02u%02u%02u%02u", m->tm_year + 1900, m->tm_mon + 1, m->tm_mday, m->tm_hour, m->tm_min, m->tm_sec); - return 1; } /* * cmd_abor(): Handle the ABOR command (abort a file transfer). This should * be clean enough, but isn't tested extensively. */ -int cmd_abor(struct conn * const c) +void cmd_abor(struct conn * const c) { if (c->transfer != NULL) { numeric(c, 426, "File transfer aborted."); destroy_ftran(c->transfer); } numeric(c, 226, "ABOR command processed OK."); - return 1; } /* * cmd_dele(): Handle the DELE command (delete a file). */ -int cmd_dele(struct conn * const c) +void cmd_dele(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); - TRAP_ERROR(fname == NULL || unlink(fname) == -1, 550, return 1); + TRAP_ERROR(fname == NULL || unlink(fname) == -1, 550, return); numeric(c, 250, "File deleted OK."); - return 1; } /* * cmd_rnfr(): Handle the RNFR command (take a filename to rename from). */ -int cmd_rnfr(struct conn * const c) +void cmd_rnfr(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); char cwd[256]; struct stat buf; c->rename_from[0] = '\0'; - if (fname == NULL) return 1; + if (fname == NULL) return; getcwd(cwd, 256); snprintf(c->rename_from, 256, "%s/%s", cwd, fname); /* Just check that the file exists. */ - TRAP_ERROR(lstat(c->rename_from, &buf) == -1, 550, c->rename_from[0] = '\0'; return 1); + TRAP_ERROR(lstat(c->rename_from, &buf) == -1, 550, c->rename_from[0] = '\0'; return); numeric(c, 350, "File exists, send RNTO."); - return 1; } /* * cmd_rnto(): Handle the RNTO command (do the actual renaming). */ -int cmd_rnto(struct conn * const c) +void cmd_rnto(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); - if (fname == NULL) return 1; + if (fname == NULL) return; if (c->rename_from[0] == '\0') { numeric(c, 503, "Please send RNFR first."); - return 1; + return; } - TRAP_ERROR(rename(c->rename_from, fname) == -1, 550, c->rename_from[0] = '\0'; return 1); + TRAP_ERROR(rename(c->rename_from, fname) == -1, 550, c->rename_from[0] = '\0'; return); c->rename_from[0] = '\0'; numeric(c, 250, "File renamed successfully."); - return 1; } /* @@ -842,18 +825,20 @@ int cmd_rnto(struct conn * const c) * easy :-) (This code isn't quite easy to understand, because * temp2 is used twice, in two different roles.) */ -int cmd_mkd(struct conn * const c) +void cmd_mkd(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); char temp[512], temp2[1024], *cdir; int i, j; - TRAP_ERROR(fname == NULL || mkdir(fname, 0755) == -1, 550, return 1); + TRAP_ERROR(fname == NULL || mkdir(fname, 0755) == -1, 550, return); chdir(fname); getcwd(temp2, 512); cdir = do_pwd(c, temp, temp2); + if (do_pwd == NULL) return; + /* double the quotes in the output */ for (i = 0, j = 0; i <= strlen(cdir); i++, j++) { temp2[j] = cdir[i]; @@ -862,20 +847,18 @@ int cmd_mkd(struct conn * const c) } } numeric(c, 257, "\"%s\" created.", temp2); - return 1; } /* * cmd_rmd(): Handle the RMD/XRMD command. Works just like DELE, only for * directories. */ -int cmd_rmd(struct conn * const c) +void cmd_rmd(struct conn * const c) { const char * const fname = translate_path(c, c->recv_buf); - TRAP_ERROR(fname == NULL || rmdir(fname) == -1, 550, return 1); + TRAP_ERROR(fname == NULL || rmdir(fname) == -1, 550, return); numeric(c, 250, "Directory deleted."); - return 1; } /* @@ -889,10 +872,9 @@ int cmd_rmd(struct conn * const c) * to the full-screen mode, but close to no FTP clients send this * command, and it would touch too much code. */ -int cmd_allo(struct conn * const c) +void cmd_allo(struct conn * const c) { numeric(c, 202, "No storage allocation necessary."); - return 1; } /* @@ -922,7 +904,7 @@ char ftran_state[6][42] = { }; #endif -int cmd_stat(struct conn * const c) +void cmd_stat(struct conn * const c) { #if WANT_STAT char buf[1024]; @@ -951,13 +933,11 @@ int cmd_stat(struct conn * const c) err = send(c->sock, buf, i, 0); if (err == -1 && errno == EPIPE) { - destroy_conn(c); - return 0; + this->free_me = 1; } #else numeric(c, 502, "STAT command disabled for security reasons."); #endif - return 1; } #if HAVE_MMAP @@ -1099,7 +1079,6 @@ int long_listing(char * const retbuf, const char * const pathname, const int do_ } strcpy(retbuf, temp); - return 1; } /* @@ -1107,7 +1086,7 @@ int long_listing(char * const retbuf, const char * const pathname, const int do_ * long listing (of type `ls -l'). The listing work is * done by do_listing(), below. */ -int cmd_list(struct conn * const c) +void cmd_list(struct conn * const c) { struct list_options lo; @@ -1116,7 +1095,6 @@ int cmd_list(struct conn * const c) lo.classify = 0; do_listing(c, &lo); - return 1; } /* @@ -1126,7 +1104,7 @@ int cmd_list(struct conn * const c) * FTP clients don't have a clue about what they send out). * The listing work is done by do_listing(), below. */ -int cmd_nlst(struct conn * const c) +void cmd_nlst(struct conn * const c) { struct list_options lo; @@ -1135,7 +1113,6 @@ int cmd_nlst(struct conn * const c) lo.classify = 0; do_listing(c, &lo); - return 1; } /* @@ -1424,25 +1401,23 @@ int list_core(struct conn * const c, const char * const pathname, * cmd_noop(): Handles the NOOP command. Does nothing, doesn't even * reset the timeout. */ -int cmd_noop(struct conn * const c) +void cmd_noop(struct conn * const c) { numeric(c, 200, "NOOP command successful."); - return 1; } /* * cmd_syst(): Handles the SYST command. Returns the system identification. */ -int cmd_syst(struct conn * const c) +void cmd_syst(struct conn * const c) { numeric(c, 215, "UNIX Type: L%u", NBBY); - return 1; } /* * cmd_type(): Handles the TYPE command. */ -int cmd_type(struct conn * const c) +void cmd_type(struct conn * const c) { #if WANT_ASCII c->recv_buf[0] &= (255-32); /* convert to upper case */ @@ -1458,13 +1433,12 @@ int cmd_type(struct conn * const c) #else numeric(c, 200, "TYPE ignored (always I)"); #endif - return 1; } /* * cmd_mode(): Handles the MODE command. Only stream mode is supported. */ -int cmd_mode(struct conn * const c) +void cmd_mode(struct conn * const c) { c->recv_buf[0] &= (255-32); /* convert to upper case */ if (c->recv_buf[0] == 'S') { @@ -1472,13 +1446,12 @@ int cmd_mode(struct conn * const c) } else { numeric(c, 504, "Unknown mode."); } - return 1; } /* * cmd_stru(): Handles the STRU command. Only file mode is supported. */ -int cmd_stru(struct conn * const c) +void cmd_stru(struct conn * const c) { c->recv_buf[0] &= (255-32); /* convert to upper case */ if (c->recv_buf[0] == 'F') { @@ -1486,7 +1459,6 @@ int cmd_stru(struct conn * const c) } else { numeric(c, 504, "Unknown structure."); } - return 1; } /* @@ -1506,21 +1478,19 @@ int cmd_stru(struct conn * const c) * like an error, since the error code is intended for helpful * messages? :-) */ -int cmd_help(struct conn * const c) +void cmd_help(struct conn * const c) { numeric(c, 414, "Sorry, no detailed help; use standard FTP commands."); - return 1; } /* * cmd_quit(): Handles the QUIT command, which shuts down the control * and data sockets. */ -int cmd_quit(struct conn * const c) +void cmd_quit(struct conn * const c) { numeric(c, 221, "Have a nice day!"); - destroy_conn(c); - return 0; + c->free_me = 1; } /* @@ -1529,7 +1499,7 @@ int cmd_quit(struct conn * const c) * copied directly from alloc_new_conn() -- perhaps we should * modularize this? */ -int cmd_rein(struct conn * const c) +void cmd_rein(struct conn * const c) { destroy_ftran(c->transfer); c->buf_len = c->auth = c->rest_pos = 0; @@ -1544,8 +1514,6 @@ int cmd_rein(struct conn * const c) time(&(c->last_transfer)); numeric(c, 220, "BetaFTPD " VERSION " ready."); - - return 1; } #if DOING_PROFILING @@ -1559,7 +1527,7 @@ int cmd_rein(struct conn * const c) * down without clearing any sockets etc. In other words: * Don't use it on a production site. */ -int cmd_exit(struct conn * const c) +void cmd_exit(struct conn * const c) { while (first_conn->next_conn) destroy_conn(first_conn->next_conn); @@ -1626,9 +1594,20 @@ void parse_command(struct conn *c) schar = c->recv_buf[cmlen]; c->recv_buf[cmlen] = 0; - - /* result of zero means the connection is freed */ - if (h->callback(c)) { + + message_buf[0] = '\0'; + h->callback(c); + if (message_buf[0] != '\0') { + /* send any feedback we might have */ + int err = send(c->sock, message_buf, strlen(message_buf), 0); + if (err == -1 && errno == EPIPE) { + c->free_me = 1; + } + } + + if (c->free_me) { + destroy_conn(c); + } else { c->recv_buf[cmlen] = schar; #if !WANT_NONROOT if (h->do_setuid) { diff --git a/cmds.h b/cmds.h index 4b107ea..8075f1c 100644 --- a/cmds.h +++ b/cmds.h @@ -54,7 +54,7 @@ } #endif -#define CMD_PROTO(cmd) int cmd_ ## cmd (struct conn * const c) +#define CMD_PROTO(cmd) void cmd_ ## cmd (struct conn * const c) int do_chdir(struct conn * const c, const char * const newd); CMD_PROTO(user); diff --git a/ftpd.c b/ftpd.c index 812a957..23a7134 100644 --- a/ftpd.c +++ b/ftpd.c @@ -184,6 +184,7 @@ struct ftran *first_ftran = NULL; #if WANT_DCACHE struct dcache *first_dcache = NULL; #endif +char message_buf[512]; #if HAVE_POLL unsigned int highest_fds = 0; @@ -365,7 +366,7 @@ struct conn *alloc_new_conn(const int sock) const unsigned int one = 1; struct conn *c = (struct conn *)(malloc(sizeof(struct conn))); - if (c == NULL) return c; + if (c == NULL) return NULL; if (sock != -1) { ioctl(sock, FIONBIO, &one); @@ -390,6 +391,7 @@ struct conn *alloc_new_conn(const int sock) #if WANT_ASCII c->ascii_mode = 0; #endif + c->free_me = 0; /* * equals: @@ -406,7 +408,7 @@ struct conn *alloc_new_conn(const int sock) time(&(c->last_transfer)); - /*list_clients();*/ + /* list_clients(); */ return c; } @@ -596,7 +598,12 @@ int process_all_clients(const fd_set * const active_clients, const int num_ac) */ void finish_transfer(struct ftran * const f) { - numeric(f->owner, 226, "Transfer complete."); + char finished[] = "226 Transfer complete.\r\n"; + if (send(f->owner->sock, finished, sizeof(finished), 0) == -1 && errno == EPIPE) { + destroy_conn(f->owner); + return; + } + time(&(f->owner->last_transfer)); #if WANT_XFERLOG @@ -1117,10 +1124,14 @@ void accept_new_client(int * const server_sock) struct conn * const c = alloc_new_conn(tempsock); num_err = 0; if (c != NULL) { - numeric(c, 220, "BetaFTPD " VERSION " ready."); + char hello[] = "220 BetaFTPD " VERSION " ready.\r\n"; + #if WANT_STAT memcpy(&(c->addr), &tempaddr, sizeof(struct sockaddr)); #endif + + if (send(tempsock, hello, sizeof(hello), 0) == -1 && errno == EPIPE) + destroy_conn(c); } } } @@ -1192,23 +1203,26 @@ void remove_bytes(struct conn * const c, const int num) * you can use this command much the same way as you * would use a printf() (with all the normal %s, %d, * etc.), since it actually uses printf() internally. + * + * This command doesn't actually SEND the data -- it + * just puts it in a buffer which is sent after the + * command handler has completed. The reasons for this + * are simple -- it makes error checking and cleanup + * MUCH cleaner, so we won't have to check for errors + * in every single little branch of the code. */ void numeric(struct conn * const c, const int numeric, const char * const format, ...) { - char buf[256], fmt[256]; + char fmt[256]; va_list args; int i, err; + int in_buf = strlen(message_buf); snprintf(fmt, 256, "%03u %s\r\n", numeric, format); va_start(args, format); - i = vsnprintf(buf, 256, fmt, args); + i = vsnprintf(message_buf + in_buf, 512 - in_buf, fmt, args); va_end(args); - - err = send(c->sock, buf, i, 0); - if (err == -1 && errno == EPIPE) { - destroy_conn(c); - } } /* diff --git a/ftpd.h b/ftpd.h index 7a165dc..835f4d7 100644 --- a/ftpd.h +++ b/ftpd.h @@ -76,6 +76,8 @@ #undef WANT_DCACHE #endif +extern char message_buf[]; + struct list_options { int recursive; int long_listing; @@ -128,6 +130,7 @@ struct conn { #endif time_t last_transfer; + int free_me; }; /* doubly linked list of file transfers */