From e3e66eec0c7051bf431d437a86de9c341bef0600 Mon Sep 17 00:00:00 2001 From: yar Date: Wed, 19 Jan 2005 10:33:20 +0000 Subject: Improve handling SIGURG and OOB commands on the control channel. The major change is to process STAT sent as an OOB command w/o breaking the current data transfer. As a side effect, this gives better error checking in the code performing data transfers. A lesser, but in no way cosmetic, change is using the flag `recvurg' in the only signal-safe way that has been blessed by SUSv3. The other flag, `transflag,' becomes private to the SIGURG machinery, serves debugging purposes only, and may be dropped in the future. The `byte_count' global variable is now accounting bytes actually transferred over the network. This can give status messages looking strange, like "X of Y bytes transferred," where X > Y, but that has more sense than trying to compensate for combinations of data formats on the server and client when transferring ASCII type data. BTW, getting the size of a file in advance is unreliable for a number of reasons in the first place. See question 18.8 of the Infrequently Asked Questions in comp.lang.c for details. PR: bin/52072 Tested by: Nick Leuta (earlier versions), a stress-testing tool (final) MFC after: 1 month --- libexec/ftpd/ftpd.c | 403 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 269 insertions(+), 134 deletions(-) diff --git a/libexec/ftpd/ftpd.c b/libexec/ftpd/ftpd.c index 62257e8..51332db 100644 --- a/libexec/ftpd/ftpd.c +++ b/libexec/ftpd/ftpd.c @@ -147,8 +147,6 @@ int noguestretr = 0; /* RETR command is disabled for anon users. */ int noguestmkd = 0; /* MKD command is disabled for anon users. */ int noguestmod = 1; /* anon users may not modify existing files. */ -static volatile sig_atomic_t recvurg; -sig_atomic_t transflag; off_t file_size; off_t byte_count; #if !defined(CMASK) || CMASK == 0 @@ -221,13 +219,34 @@ char proctitle[LINE_MAX]; /* initial part of title */ #define LOGCMD2(cmd, file1, file2) logcmd((cmd), (file1), (file2), -1) #define LOGBYTES(cmd, file, cnt) logcmd((cmd), (file), NULL, (cnt)) +static volatile sig_atomic_t recvurg; +static int transflag; /* NB: for debugging only */ + +#define STARTXFER flagxfer(1) +#define ENDXFER flagxfer(0) + +#define START_UNSAFE maskurg(1) +#define END_UNSAFE maskurg(0) + +/* It's OK to put an `else' clause after this macro. */ +#define CHECKOOB(action) \ + if (recvurg) { \ + recvurg = 0; \ + if (myoob()) { \ + ENDXFER; \ + action; \ + } \ + } + #ifdef VIRTUAL_HOSTING static void inithosts(void); static void selecthost(union sockunion *); #endif static void ack(char *); static void sigurg(int); -static void myoob(void); +static void maskurg(int); +static void flagxfer(int); +static int myoob(void); static int checkuser(char *, char *, int, char **); static FILE *dataconn(char *, off_t, char *); static void dolog(struct sockaddr *); @@ -1982,6 +2001,30 @@ pdata_err: } /* + * A helper macro to avoid code duplication + * in send_data() and receive_data(). + * + * XXX We have to block SIGURG during putc() because BSD stdio + * is unable to restart interrupted write operations and hence + * the entire buffer contents will be lost as soon as a write() + * call indicates EINTR to stdio. + */ +#define FTPD_PUTC(ch, file, label) \ + do { \ + int ret; \ + \ + do { \ + START_UNSAFE; \ + ret = putc((ch), (file)); \ + END_UNSAFE; \ + CHECKOOB(return (-1)) \ + else if (ferror(file)) \ + goto label; \ + clearerr(file); \ + } while (ret == EOF); \ + } while (0) + +/* * Tranfer the contents of "instr" to "outstr" peer using the appropriate * encapsulation of the data subject to Mode, Structure, and Type. * @@ -1992,33 +2035,50 @@ send_data(FILE *instr, FILE *outstr, size_t blksize, off_t filesize, int isreg) { int c, cp, filefd, netfd; char *buf; - off_t cnt; - transflag++; + STARTXFER; + switch (type) { case TYPE_A: - cp = '\0'; - while ((c = getc(instr)) != EOF) { - if (recvurg) - goto got_oob; - byte_count++; + cp = EOF; + for (;;) { + c = getc(instr); + CHECKOOB(return (-1)) + else if (c == EOF && ferror(instr)) + goto file_err; + if (c == EOF) { + if (ferror(instr)) { /* resume after OOB */ + clearerr(instr); + continue; + } + if (feof(instr)) /* EOF */ + break; + syslog(LOG_ERR, "Internal: impossible condition" + " on file after getc()"); + goto file_err; + } if (c == '\n' && cp != '\r') { - if (ferror(outstr)) - goto data_err; - (void) putc('\r', outstr); + FTPD_PUTC('\r', outstr, data_err); + byte_count++; } - (void) putc(c, outstr); + FTPD_PUTC(c, outstr, data_err); + byte_count++; cp = c; } - if (recvurg) - goto got_oob; - fflush(outstr); - transflag = 0; - if (ferror(instr)) - goto file_err; - if (ferror(outstr)) +#ifdef notyet /* BSD stdio isn't ready for that */ + while (fflush(outstr) == EOF) { + CHECKOOB(return (-1)) + else + goto data_err; + clearerr(outstr); + } + ENDXFER; +#else + ENDXFER; + if (fflush(outstr) == EOF) goto data_err; +#endif reply(226, "Transfer complete."); return (0); @@ -2033,7 +2093,7 @@ send_data(FILE *instr, FILE *outstr, size_t blksize, off_t filesize, int isreg) if (isreg) { char *msg = "Transfer complete."; - off_t offset; + off_t cnt, offset; int err; cnt = offset = 0; @@ -2046,18 +2106,17 @@ send_data(FILE *instr, FILE *outstr, size_t blksize, off_t filesize, int isreg) * It can be used in myoob() later. */ byte_count += cnt; - if (recvurg) - goto got_oob; offset += cnt; filesize -= cnt; - - if (err == -1) { - if (cnt == 0 && offset == 0) + CHECKOOB(return (-1)) + else if (err == -1) { + if (errno != EINTR && + cnt == 0 && offset == 0) goto oldway; - goto data_err; } - + if (err == -1) /* resume after OOB */ + continue; /* * We hit the EOF prematurely. * Perhaps the file was externally truncated. @@ -2068,52 +2127,65 @@ send_data(FILE *instr, FILE *outstr, size_t blksize, off_t filesize, int isreg) break; } } - - transflag = 0; + ENDXFER; reply(226, msg); return (0); } oldway: if ((buf = malloc(blksize)) == NULL) { - transflag = 0; + ENDXFER; reply(451, "Ran out of memory."); return (-1); } - while ((cnt = read(filefd, buf, blksize)) > 0 && - write(netfd, buf, cnt) == cnt) - byte_count += cnt; - transflag = 0; - free(buf); - if (cnt != 0) { - if (cnt < 0) + for (;;) { + int cnt, len; + char *bp; + + cnt = read(filefd, buf, blksize); + CHECKOOB(free(buf); return (-1)) + else if (cnt < 0) { + free(buf); goto file_err; - goto data_err; + } + if (cnt < 0) /* resume after OOB */ + continue; + if (cnt == 0) /* EOF */ + break; + for (len = cnt, bp = buf; len > 0;) { + cnt = write(netfd, bp, len); + CHECKOOB(free(buf); return (-1)) + else if (cnt < 0) { + free(buf); + goto data_err; + } + if (cnt <= 0) + continue; + len -= cnt; + bp += cnt; + byte_count += cnt; + } } + ENDXFER; + free(buf); reply(226, "Transfer complete."); return (0); default: - transflag = 0; + ENDXFER; reply(550, "Unimplemented TYPE %d in send_data.", type); return (-1); } data_err: - transflag = 0; + ENDXFER; perror_reply(426, "Data connection"); return (-1); file_err: - transflag = 0; + ENDXFER; perror_reply(551, "Error on input file"); return (-1); - -got_oob: - myoob(); - recvurg = 0; - transflag = 0; - return (-1); } /* @@ -2125,63 +2197,98 @@ got_oob: static int receive_data(FILE *instr, FILE *outstr) { - int c; - int cnt, bare_lfs; - char buf[BUFSIZ]; + int c, cp; + int bare_lfs = 0; - transflag++; - bare_lfs = 0; + STARTXFER; switch (type) { case TYPE_I: case TYPE_L: - while ((cnt = read(fileno(instr), buf, sizeof(buf))) > 0) { - if (recvurg) - goto got_oob; - if (write(fileno(outstr), buf, cnt) != cnt) - goto file_err; - byte_count += cnt; + for (;;) { + int cnt, len; + char *bp; + char buf[BUFSIZ]; + + cnt = read(fileno(instr), buf, sizeof(buf)); + CHECKOOB(return (-1)) + else if (cnt < 0) + goto data_err; + if (cnt < 0) /* resume after OOB */ + continue; + if (cnt == 0) /* EOF */ + break; + for (len = cnt, bp = buf; len > 0;) { + cnt = write(fileno(outstr), bp, len); + CHECKOOB(return (-1)) + else if (cnt < 0) + goto file_err; + if (cnt <= 0) + continue; + len -= cnt; + bp += cnt; + byte_count += cnt; + } } - if (recvurg) - goto got_oob; - if (cnt < 0) - goto data_err; - transflag = 0; + ENDXFER; return (0); case TYPE_E: + ENDXFER; reply(553, "TYPE E not implemented."); - transflag = 0; return (-1); case TYPE_A: - while ((c = getc(instr)) != EOF) { - if (recvurg) - goto got_oob; - byte_count++; - if (c == '\n') - bare_lfs++; - while (c == '\r') { - if (ferror(outstr)) - goto data_err; - if ((c = getc(instr)) != '\n') { - (void) putc ('\r', outstr); - if (c == '\0' || c == EOF) - goto contin2; - } + cp = EOF; + for (;;) { + c = getc(instr); + CHECKOOB(return (-1)) + else if (c == EOF && ferror(instr)) + goto data_err; + if (c == EOF && ferror(instr)) { /* resume after OOB */ + clearerr(instr); + continue; } - (void) putc(c, outstr); - contin2: ; + + if (cp == '\r') { + if (c != '\n') + FTPD_PUTC('\r', outstr, file_err); + } else + if (c == '\n') + bare_lfs++; + if (c == '\r') { + byte_count++; + cp = c; + continue; + } + + /* Check for EOF here in order not to lose last \r. */ + if (c == EOF) { + if (feof(instr)) /* EOF */ + break; + syslog(LOG_ERR, "Internal: impossible condition" + " on data stream after getc()"); + goto data_err; + } + + byte_count++; + FTPD_PUTC(c, outstr, file_err); + cp = c; } - if (recvurg) - goto got_oob; - fflush(outstr); - if (ferror(instr)) - goto data_err; - if (ferror(outstr)) +#ifdef notyet /* BSD stdio isn't ready for that */ + while (fflush(outstr) == EOF) { + CHECKOOB(return (-1)) + else + goto file_err; + clearerr(outstr); + } + ENDXFER; +#else + ENDXFER; + if (fflush(outstr) == EOF) goto file_err; - transflag = 0; +#endif if (bare_lfs) { lreply(226, "WARNING! %d bare linefeeds received in ASCII mode.", @@ -2190,26 +2297,20 @@ receive_data(FILE *instr, FILE *outstr) } return (0); default: + ENDXFER; reply(550, "Unimplemented TYPE %d in receive_data.", type); - transflag = 0; return (-1); } data_err: - transflag = 0; + ENDXFER; perror_reply(426, "Data connection"); return (-1); file_err: - transflag = 0; + ENDXFER; perror_reply(452, "Error writing to file"); return (-1); - -got_oob: - myoob(); - recvurg = 0; - transflag = 0; - return (-1); } void @@ -2625,11 +2726,6 @@ dolog(struct sockaddr *who) void dologout(int status) { - /* - * Prevent reception of SIGURG from resulting in a resumption - * back to the main program loop. - */ - transflag = 0; if (logged_in && dowtmp) { (void) seteuid(0); @@ -2647,13 +2743,46 @@ sigurg(int signo) } static void +maskurg(int flag) +{ + int oerrno; + sigset_t sset; + + if (!transflag) { + syslog(LOG_ERR, "Internal: maskurg() while no transfer"); + return; + } + oerrno = errno; + sigemptyset(&sset); + sigaddset(&sset, SIGURG); + sigprocmask(flag ? SIG_BLOCK : SIG_UNBLOCK, &sset, NULL); + errno = oerrno; +} + +static void +flagxfer(int flag) +{ + + maskurg(!flag); + if (flag) { + recvurg = 0; + transflag = 1; + } else + transflag = 0; +} + +/* + * Returns 0 if OK to resume or -1 if abort requested. + */ +static int myoob(void) { char *cp; - /* only process if transfer occurring */ - if (!transflag) - return; + if (!transflag) { + syslog(LOG_ERR, "Internal: myoob() while no transfer"); + return (0); + } cp = tmpline; if (getline(cp, 7, stdin) == NULL) { reply(221, "You could at least say goodbye."); @@ -2664,6 +2793,7 @@ myoob(void) tmpline[0] = '\0'; reply(426, "Transfer aborted. Data connection closed."); reply(226, "Abort successful."); + return (-1); } if (strcmp(cp, "STAT\r\n") == 0) { tmpline[0] = '\0'; @@ -2674,6 +2804,7 @@ myoob(void) reply(213, "Status: %jd bytes transferred.", (intmax_t)byte_count); } + return (0); } /* @@ -3016,17 +3147,10 @@ send_file_list(char *whichf) * used NLST, do what the user meant. */ if (dirname[0] == '-' && *dirlist == NULL && - transflag == 0) { + dout == NULL) retrieve(_PATH_LS " %s", dirname); - goto out; - } - perror_reply(550, whichf); - if (dout != NULL) { - (void) fclose(dout); - transflag = 0; - data = -1; - pdata = -1; - } + else + perror_reply(550, whichf); goto out; } @@ -3035,11 +3159,17 @@ send_file_list(char *whichf) dout = dataconn("file list", -1, "w"); if (dout == NULL) goto out; - transflag++; + STARTXFER; } + START_UNSAFE; fprintf(dout, "%s%s\n", dirname, type == TYPE_A ? "\r" : ""); - byte_count += strlen(dirname) + 1; + END_UNSAFE; + if (ferror(dout)) + goto data_err; + byte_count += strlen(dirname) + + (type == TYPE_A ? 2 : 1); + CHECKOOB(goto abrt); continue; } else if (!S_ISDIR(st.st_mode)) continue; @@ -3050,12 +3180,7 @@ send_file_list(char *whichf) while ((dir = readdir(dirp)) != NULL) { char nbuf[MAXPATHLEN]; - if (recvurg) { - myoob(); - recvurg = 0; - transflag = 0; - goto out; - } + CHECKOOB(goto abrt); if (dir->d_name[0] == '.' && dir->d_namlen == 1) continue; @@ -3076,33 +3201,43 @@ send_file_list(char *whichf) dout = dataconn("file list", -1, "w"); if (dout == NULL) goto out; - transflag++; + STARTXFER; } + START_UNSAFE; if (nbuf[0] == '.' && nbuf[1] == '/') fprintf(dout, "%s%s\n", &nbuf[2], type == TYPE_A ? "\r" : ""); else fprintf(dout, "%s%s\n", nbuf, type == TYPE_A ? "\r" : ""); - byte_count += strlen(nbuf) + 1; + END_UNSAFE; + if (ferror(dout)) + goto data_err; + byte_count += strlen(nbuf) + + (type == TYPE_A ? 2 : 1); + CHECKOOB(goto abrt); } } (void) closedir(dirp); + dirp = NULL; } if (dout == NULL) reply(550, "No files found."); - else if (ferror(dout) != 0) - perror_reply(550, "Data connection"); + else if (ferror(dout)) +data_err: perror_reply(550, "Data connection"); else reply(226, "Transfer complete."); - - transflag = 0; - if (dout != NULL) - (void) fclose(dout); - data = -1; - pdata = -1; out: + if (dout) { + ENDXFER; +abrt: + (void) fclose(dout); + data = -1; + pdata = -1; + } + if (dirp) + (void) closedir(dirp); if (freeglob) { freeglob = 0; globfree(&gl); -- cgit v1.1