2007-02-08 Marcus Brinkmann <marcus@g10code.de>
authorMarcus Brinkmann <mb@g10code.com>
Thu, 8 Feb 2007 00:29:42 +0000 (00:29 +0000)
committerMarcus Brinkmann <mb@g10code.com>
Thu, 8 Feb 2007 00:29:42 +0000 (00:29 +0000)
* src/telnet_session.c: Include <pthread.h>.
(process_data): Enable cancellation.
* src/ftp_session.c (netscape_hack): Likewise.
(send_readme, do_list, do_nlst): Use cancellation cleanup handler.
(set_pasv): Release socket_fd before invoking reply() in case of
failure.
(open_connection): Likewise.
(do_pasv): Close outstanding passive port before invoking reply().
(do_lpsv): Likewise.
(do_epsv): Likewise.
* src/file_list.c: Always include <pthread.h>.
(fdprintf): Bump buffer size to PATH_MAX + 3.  Enable cancellation
while writing.
* src/ftp_listener.c (connection_handler): Do not use asynchronous
cancellation.  Disable cancellation until further notice.

ChangeLog
src/file_list.c
src/ftp_listener.c
src/ftp_session.c
src/telnet_session.c

index edaa6f9..e7265a4 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2007-02-08  Marcus Brinkmann  <marcus@g10code.de>
+
+       * src/telnet_session.c: Include <pthread.h>.
+       (process_data): Enable cancellation.
+       * src/ftp_session.c (netscape_hack): Likewise.
+       (send_readme, do_list, do_nlst): Use cancellation cleanup handler.
+       (set_pasv): Release socket_fd before invoking reply() in case of
+       failure.
+       (open_connection): Likewise.
+       (do_pasv): Close outstanding passive port before invoking reply().
+       (do_lpsv): Likewise.
+       (do_epsv): Likewise.
+       * src/file_list.c: Always include <pthread.h>.
+       (fdprintf): Bump buffer size to PATH_MAX + 3.  Enable cancellation
+       while writing.
+       * src/ftp_listener.c (connection_handler): Do not use asynchronous
+       cancellation.  Disable cancellation until further notice.
+
 2007-02-06  Marcus Brinkmann  <marcus@g10code.de>
 
        * configure.in (AC_CHECK_HEADERS): Check for sys/sendfile.h.
index 8482b52..8824760 100644 (file)
@@ -14,6 +14,7 @@
 #include <stdlib.h>
 #include <stdarg.h>
 #include <ctype.h>
+#include <pthread.h>
 
 #if TIME_WITH_SYS_TIME
 # include <sys/time.h>
@@ -62,7 +63,6 @@ static const char *skip_ls_options(const char *filespec);
 
 /* if no localtime_r() is available, provide one */
 #ifndef HAVE_LOCALTIME_R
-#include <pthread.h>
 
 struct tm *localtime_r(const time_t *timep, struct tm *timeptr) 
 {
@@ -75,6 +75,11 @@ struct tm *localtime_r(const time_t *timep, struct tm *timeptr)
 }
 #endif /* HAVE_LOCALTIME_R */
 
+static void file_nlst_cleanup(glob_t *glob_buf)
+{
+  globfree (glob_buf);
+}
+
 int file_nlst(int out, const char *cur_dir, const char *filespec)
 {
     int dir_len;
@@ -139,6 +144,8 @@ int file_nlst(int out, const char *cur_dir, const char *filespec)
        return 0;
     }
 
+    pthread_cleanup_push ((void (*)())file_nlst_cleanup, &glob_buf);
+
     /* print our results */
     for (i=0; i<glob_buf.gl_pathc; i++) {
         file_name = glob_buf.gl_pathv[i];
@@ -151,8 +158,9 @@ int file_nlst(int out, const char *cur_dir, const char *filespec)
        fdprintf(out, "%s\r\n", file_name);
     }
 
-    /* free and return */
-    globfree(&glob_buf);
+    /* This frees glob_buf.  */
+    pthread_cleanup_pop(1);
+
     return 1;
 }
 
@@ -162,6 +170,20 @@ typedef struct {
     struct stat stat;
 } file_info_t;
 
+struct file_list_cleanup_s
+{
+  void *file_info;
+  glob_t *glob_buf;
+};
+
+static void file_list_cleanup(struct file_list_cleanup_s *info)
+{
+  if (info->file_info)
+    free (info->file_info);
+  if (info->glob_buf)
+    globfree (info->glob_buf);
+}
+  
 int file_list(int out, const char *cur_dir, const char *filespec)
 {
     int dir_len;
@@ -181,7 +203,12 @@ int file_list(int out, const char *cur_dir, const char *filespec)
     char date_buf[13];
     char link[PATH_MAX+1];
     int link_len;
-    
+
+    struct file_list_cleanup_s cleanup_info;
+
+    cleanup_info.file_info = NULL;
+    cleanup_info.glob_buf = NULL;
+
     daemon_assert(out >= 0);
     daemon_assert(is_valid_dir(cur_dir));
     daemon_assert(filespec != NULL);
@@ -240,18 +267,24 @@ int file_list(int out, const char *cur_dir, const char *filespec)
        return 0;
     }
 
     /* make a buffer to store our information */
 #ifdef HAVE_ALLOCA
     file_info = (file_info_t *)alloca(sizeof(file_info_t) * glob_buf.gl_pathc);
 #else
     file_info = (file_info_t *)malloc(sizeof(file_info_t) * glob_buf.gl_pathc);
+    cleanup_info.file_info = file_info;
 #endif
     if (file_info == NULL) {
-        fdprintf(out, "Error; Out of memory\r\n");
+       /* Note that fdprintf is a cancellation point.  */
        globfree(&glob_buf);
+        fdprintf(out, "Error; Out of memory\r\n");
        return 0;
     }
 
+    cleanup_info.glob_buf = &glob_buf;
+    pthread_cleanup_push ((void (*)())file_list_cleanup, &cleanup_info);
+
     /* collect information */
     num_files = 0;
     total_blocks = 0;
@@ -369,11 +402,9 @@ int file_list(int out, const char *cur_dir, const char *filespec)
        fdprintf(out, "\r\n");
     }
 
-    /* free memory & return */
-#ifndef HAVE_ALLOCA
-    free(file_info);
-#endif 
-    globfree(&glob_buf);
+    /* This frees file_info (if necessary) and glob_buf.  */
+    pthread_cleanup_pop (1);
+
     return 1;
 }
 
@@ -400,18 +431,18 @@ static int is_valid_dir(const char *dir)
 
 static void fdprintf(int fd, const char *fmt, ...)
 {
-    char buf[PATH_MAX+1];
+    char buf[PATH_MAX+3];
     int buflen;
     va_list ap;
     int amt_written;
     int write_ret;
+    int state;
 
     daemon_assert(fd >= 0);
     daemon_assert(fmt != NULL);
 
     va_start(ap, fmt);
     buflen = vsnprintf(buf, sizeof(buf)-1, fmt, ap);
-    buf[PATH_MAX] = 0; /* Make extra sure that the string is terminated. */
     va_end(ap);
     if (buflen <= 0) {
         return;
@@ -419,15 +450,18 @@ static void fdprintf(int fd, const char *fmt, ...)
     if (buflen >= sizeof(buf)) {
         buflen = sizeof(buf)-1;
     }
+    buf[buflen] = 0; /* Make extra sure the string is terminated. */
 
+    pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, &state);
     amt_written = 0;
     while (amt_written < buflen) {
         write_ret = write(fd, buf+amt_written, buflen-amt_written);
        if (write_ret <= 0) {
-           return;
+           break;
        }
        amt_written += write_ret;
     }
+    pthread_setcancelstate (state, NULL);
 }
 
 
index d294df1..f380577 100644 (file)
@@ -516,8 +516,9 @@ static void *connection_handler(connection_info_t *info)
     /* don't save state for pthread_join() */
     pthread_detach(pthread_self());
 
-    /* set up our watchdog */
-    pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
+    /* Set up our watchdog.  We only enable cancellation during long
+       operations.  */
+    pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
     watchdog_add_watched(&f->watchdog, &info->watched);
 
     /* set up our cleanup handler */
index 3441eee..064cf9b 100644 (file)
@@ -67,6 +67,13 @@ static void get_absolute_fname(char *fname,
                                const char *dir,
                                const char *file);
 
+/* Common cleanup handler.  */
+static void fd_cleanup (int *fd)
+{
+  if (*fd != -1)
+    close (*fd);
+}
+
 /* command handlers */
 static void do_user(ftp_session_t *f, const ftp_command_t *cmd);
 static void do_pass(ftp_session_t *f, const ftp_command_t *cmd);
@@ -746,18 +753,20 @@ static int set_pasv(ftp_session_t *f, sockaddr_storage_t *bind_addr)
        }
        if (errno != EADDRINUSE) {
             char errbuf[ERRBUF_SIZE];
+           /* Note that reply() may enable cancellation.  */
+            close(socket_fd);
             reply(f, 500, "Error binding server port; %s.",
                   strerror_r(errno, errbuf, ERRBUF_SIZE));
-            close(socket_fd);
             return -1;
        }
     }
 
     if (listen(socket_fd, 1) != 0) {
         char errbuf[ERRBUF_SIZE];
+       /* Note that reply() may enable cancellation.  */
+        close(socket_fd);
         reply(f, 500, "Error listening on server port; %s.",
               strerror_r(errno, errbuf, ERRBUF_SIZE));
-        close(socket_fd);
        return -1;
     }
 
@@ -788,6 +797,15 @@ static void do_pasv(ftp_session_t *f, const ftp_command_t *cmd)
     /* report port to client */
     addr = ntohl(f->server_ipv4_addr.sin_addr.s_addr);
     port = ntohs(f->server_ipv4_addr.sin_port);
+
+    /* Close any outstanding PASSIVE port.  Do this before invoking
+       reply(), because that may enable cancellation.  */
+    if (f->data_channel == DATA_PASSIVE) {
+      close(f->server_fd);
+    }
+    f->data_channel = DATA_PASSIVE;
+    f->server_fd = socket_fd;
+
     reply(f, 227, "Entering Passive Mode (%d,%d,%d,%d,%d,%d).",
         addr >> 24, 
        (addr >> 16) & 0xff,
@@ -796,13 +814,6 @@ static void do_pasv(ftp_session_t *f, const ftp_command_t *cmd)
         port >> 8, 
        port & 0xff);
 
-   /* close any outstanding PASSIVE port */
-   if (f->data_channel == DATA_PASSIVE) {
-       close(f->server_fd);
-   }
-   f->data_channel = DATA_PASSIVE;
-   f->server_fd = socket_fd;
-
 exit_pasv:
     daemon_assert(invariant(f));
 }
@@ -829,6 +840,13 @@ static void do_lpsv(ftp_session_t *f, const ftp_command_t *cmd)
         goto exit_lpsv;
     }
 
+    /* close any outstanding PASSIVE port */
+    if (f->data_channel == DATA_PASSIVE) {
+      close(f->server_fd);
+    }
+    f->data_channel = DATA_PASSIVE;
+    f->server_fd = socket_fd;
+
     /* report address and port to client */
 #ifdef INET6
     if (SSFAM(&f->server_addr) == AF_INET6) {
@@ -849,13 +867,6 @@ static void do_lpsv(ftp_session_t *f, const ftp_command_t *cmd)
 
     reply(f, 228, "Entering Long Passive Mode %s", addr);
 
-   /* close any outstanding PASSIVE port */
-   if (f->data_channel == DATA_PASSIVE) {
-       close(f->server_fd);
-   }
-   f->data_channel = DATA_PASSIVE;
-   f->server_fd = socket_fd;
-
 exit_lpsv:
     daemon_assert(invariant(f));
 }
@@ -907,10 +918,6 @@ static void do_epsv(ftp_session_t *f, const ftp_command_t *cmd)
         goto exit_epsv;
     }
 
-    /* report port to client */
-    reply(f, 229, "Entering Extended Passive Mode (|||%d|)", 
-        ntohs(SINPORT(&f->server_addr)));
-
     /* close any outstanding PASSIVE port */
     if (f->data_channel == DATA_PASSIVE) {
         close(f->server_fd);
@@ -918,6 +925,10 @@ static void do_epsv(ftp_session_t *f, const ftp_command_t *cmd)
     f->data_channel = DATA_PASSIVE;
     f->server_fd = socket_fd;  
 
+    /* report port to client */
+    reply(f, 229, "Entering Extended Passive Mode (|||%d|)", 
+        ntohs(SINPORT(&f->server_addr)));
+
 exit_epsv:
     daemon_assert(invariant(f));
 }
@@ -1095,6 +1106,7 @@ static void do_retr(ftp_session_t *f, const ftp_command_t *cmd)
     off_t amt_sent;
 #endif
     char errbuf[ERRBUF_SIZE];
+    int state;
 
     daemon_assert(invariant(f));
     daemon_assert(cmd != NULL);
@@ -1104,6 +1116,10 @@ static void do_retr(ftp_session_t *f, const ftp_command_t *cmd)
     file_fd = -1;
     socket_fd = -1;
 
+    pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, &state);
+    pthread_cleanup_push ((void (*)())fd_cleanup, &file_fd);
+    pthread_cleanup_push ((void (*)())fd_cleanup, &socket_fd);
+    
     /* create an absolute name for our file */
     file_name = cmd->arg[0].string;
     get_absolute_fname(full_path, sizeof(full_path), f->dir, file_name);
@@ -1247,7 +1263,7 @@ static void do_retr(ftp_session_t *f, const ftp_command_t *cmd)
 #endif  /* HAVE_SENDFILE */
     }
 
-    /* disconnect */
+    /* Disconnect eagerly.  */
     close(socket_fd);
     socket_fd = -1;
 
@@ -1279,13 +1295,12 @@ static void do_retr(ftp_session_t *f, const ftp_command_t *cmd)
       (long int) transfer_time.tv_usec);
 
 exit_retr:
+    /* Close socket_fd.  */
+    pthread_cleanup_pop (1);
+    /* Close file_fd.  */
+    pthread_cleanup_pop (1);
     f->file_offset = 0;
-    if (socket_fd != -1) {
-        close(socket_fd);
-    }
-    if (file_fd != -1) {
-        close(file_fd);
-    }
+    pthread_setcancelstate (state, NULL);
     daemon_assert(invariant(f));
 }
 
@@ -1302,28 +1317,32 @@ static void do_stor(ftp_session_t *f, const ftp_command_t *cmd)
 
 static int open_connection(ftp_session_t *f)
 {
-    int socket_fd;
+    int socket_fd = -1;
     struct sockaddr_in addr;
     unsigned addr_len;
     char errbuf[ERRBUF_SIZE];
+    int state;
 
     daemon_assert((f->data_channel == DATA_PORT) || 
                   (f->data_channel == DATA_PASSIVE));
 
+    pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, &state);
+    pthread_cleanup_push ((void (*)())fd_cleanup, &socket_fd);
     if (f->data_channel == DATA_PORT) {
         socket_fd = socket(SSFAM(&f->data_port), SOCK_STREAM, 0);
        if (socket_fd == -1) {
            reply(f, 425, "Error creating socket; %s.",
                   strerror_r(errno, errbuf, ERRBUF_SIZE));
-           return -1;
+           goto leave_oc;
        }
        if (connect(socket_fd, (struct sockaddr *)&f->data_port, 
-           sizeof(sockaddr_storage_t)) != 0)
+                        sizeof(sockaddr_storage_t)) != 0)
        {
+           close(socket_fd);
+           socket_fd = -1;
            reply(f, 425, "Error connecting; %s.",
                   strerror_r(errno, errbuf, ERRBUF_SIZE));
-           close(socket_fd);
-           return -1;
+           goto leave_oc;
        }
     } else {
         daemon_assert(f->data_channel == DATA_PASSIVE);
@@ -1333,34 +1352,41 @@ static int open_connection(ftp_session_t *f)
        if (socket_fd == -1) {
            reply(f, 425, "Error accepting connection; %s.",
                   strerror_r(errno, errbuf, ERRBUF_SIZE));
-           return -1;
+           goto leave_oc;
        }
 #ifdef INET6
         /* in IPv6, the client can connect to a channel using a different */
        /* protocol - in that case, we'll just blindly let the connection */
        /* through, otherwise verify addresses match */
-        if (SAFAM(addr) == SSFAM(&f->client_addr)) {
+       if (SAFAM(addr) == SSFAM(&f->client_addr)) {
            if (memcmp(&SINADDR(&f->client_addr), &SINADDR(&addr), 
                       sizeof(SINADDR(&addr))))
            {
+               close(socket_fd);
+               socket_fd = -1;
                reply(f, 425, 
                  "Error accepting connection; connection from invalid IP.");
-               close(socket_fd);
-               return -1;
+               goto leave_oc;
             }
        }
 #else
        if (memcmp(&f->client_addr.sin_addr, 
            &addr.sin_addr, sizeof(struct in_addr))) 
        {
+           close(socket_fd);
+           socket_fd = -1;
            reply(f, 425, 
              "Error accepting connection; connection from invalid IP.");
-           close(socket_fd);
-           return -1;
+           goto leave_oc;
        }
 #endif
     }
 
+ leave_oc:
+    /* Do not close SOCKET_FD here.  */
+    pthread_cleanup_pop (0);
+    pthread_setcancelstate (state, NULL);
+
     return socket_fd;
 }
 
@@ -1486,9 +1512,6 @@ static void do_nlst(ftp_session_t *f, const ftp_command_t *cmd)
     daemon_assert(cmd != NULL);
     daemon_assert((cmd->num_arg == 0) || (cmd->num_arg == 1));
 
-    /* set up for exit */
-    fd = -1;
-
     /* figure out what parameters to use */
     if (cmd->num_arg == 0) {
         param = "*";
@@ -1506,7 +1529,7 @@ static void do_nlst(ftp_session_t *f, const ftp_command_t *cmd)
     /* check spec passed */
     if (!filespec_is_legal(param)) {
         reply(f, 550, "Illegal filename passed.");
-       goto exit_nlst;
+       return;
     }
 
     /* ready to list */
@@ -1514,9 +1537,10 @@ static void do_nlst(ftp_session_t *f, const ftp_command_t *cmd)
 
     /* open our data connection */
     fd = open_connection(f);
-    if (fd == -1) {
-        goto exit_nlst;
-    }
+    if (fd == -1)
+      return;
+
+    pthread_cleanup_push ((void (*)())fd_cleanup, &fd);
 
     /* send any files */
     send_ok = file_nlst(fd, f->dir, param);
@@ -1530,11 +1554,8 @@ static void do_nlst(ftp_session_t *f, const ftp_command_t *cmd)
         reply(f, 451, "Error sending name list.");
     }
 
-    /* clean up and exit */
-exit_nlst:
-    if (fd != -1) {
-        close(fd);
-    }
+    pthread_cleanup_pop (1);
+
     daemon_assert(invariant(f));
 }
 
@@ -1548,9 +1569,6 @@ static void do_list(ftp_session_t *f, const ftp_command_t *cmd)
     daemon_assert(cmd != NULL);
     daemon_assert((cmd->num_arg == 0) || (cmd->num_arg == 1));
 
-    /* set up for exit */
-    fd = -1;
-
     /* figure out what parameters to use */
     if (cmd->num_arg == 0) {
         param = "*";
@@ -1568,7 +1586,7 @@ static void do_list(ftp_session_t *f, const ftp_command_t *cmd)
     /* check spec passed */
     if (!filespec_is_legal(param)) {
         reply(f, 550, "Illegal filename passed.");
-       goto exit_list;
+       return;
     }
 
     /* ready to list */
@@ -1576,9 +1594,10 @@ static void do_list(ftp_session_t *f, const ftp_command_t *cmd)
 
     /* open our data connection */
     fd = open_connection(f);
-    if (fd == -1) {
-        goto exit_list;
-    }
+    if (fd == -1)
+      return;
+
+    pthread_cleanup_push ((void (*)())fd_cleanup, &fd);
 
     /* send any files */
     send_ok = file_list(fd, f->dir, param);
@@ -1592,11 +1611,8 @@ static void do_list(ftp_session_t *f, const ftp_command_t *cmd)
         reply(f, 451, "Error sending file list.");
     }
 
-    /* clean up and exit */
-exit_list:
-    if (fd != -1) {
-        close(fd);
-    }
+    pthread_cleanup_pop (1);
+
     daemon_assert(invariant(f));
 }
 
@@ -1757,15 +1773,11 @@ static void send_readme(const ftp_session_t *f, int code)
     daemon_assert(code >= 100);
     daemon_assert(code <= 559);
 
-    /* set up for early exit */
-    fd = -1;
-
     /* verify our README wouldn't be too long */
     dir_len = strlen(f->dir);
-    if ((dir_len + 1 + sizeof(README_FILE_NAME)) > sizeof(file_name)) {
-        goto exit_send_readme;
-    }
-
+    if ((dir_len + 1 + sizeof(README_FILE_NAME)) > sizeof(file_name))
+      return;
+    
     /* create a README file name */
     strcpy(file_name, f->dir);
     strcat(file_name, "/");
@@ -1773,9 +1785,10 @@ static void send_readme(const ftp_session_t *f, int code)
 
     /* open our file */
     fd = open(file_name, O_RDONLY);
-    if (fd == -1) {
-        goto exit_send_readme;
-    }
+    if (fd == -1)
+      return;
+
+    pthread_cleanup_push ((void (*)())fd_cleanup, &fd);
 
     /* verify this isn't a directory */
     if (fstat(fd, &stat_buf) != 0) {
@@ -1823,9 +1836,8 @@ static void send_readme(const ftp_session_t *f, int code)
 
     /* cleanup and exit */
 exit_send_readme:
-    if (fd != -1) {
-        close(fd);
-    }
+    pthread_cleanup_pop (1);
+
     daemon_assert(invariant(f));
 }
 
@@ -1836,9 +1848,11 @@ static void netscape_hack(int fd)
     struct timeval timeout;
     int select_ret;
     char c;
+    int state;
 
     daemon_assert(fd >= 0);
 
+    pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, &state);
     shutdown(fd, 1);
     FD_ZERO(&readfds);
     FD_SET(fd, &readfds);
@@ -1848,6 +1862,7 @@ static void netscape_hack(int fd)
     if (select_ret > 0) {
         read(fd, &c, 1);
     }
+    pthread_setcancelstate (state, NULL);
 }
 
 /* compare two addresses to see if they contain the same IP address */
index 5221b0d..2523980 100644 (file)
@@ -19,6 +19,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <string.h>
+#include <pthread.h>
 #include "daemon_assert.h"
 #include "telnet_session.h"
 
@@ -197,6 +198,7 @@ static void process_data(telnet_session_t *t, int wait_flag)
     fd_set exceptfds;
     struct timeval tv_zero;
     struct timeval *tv;
+    int state;
 
     /* set up our select() variables */
     FD_ZERO(&readfds);
@@ -225,6 +227,8 @@ static void process_data(telnet_session_t *t, int wait_flag)
         FD_SET(t->out_fd, &exceptfds);
     }
 
+    pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, &state);
+
     /* see if there's anything to do */
     if (select(FD_SETSIZE, &readfds, &writefds, &exceptfds, tv) > 0) {
 
@@ -240,6 +244,8 @@ static void process_data(telnet_session_t *t, int wait_flag)
            write_outgoing_data(t);
         }
     }
+
+    pthread_setcancelstate (state, NULL);
 }
 
 static void read_incoming_data(telnet_session_t *t)