Re: [1/2] git commit: [TS-2822] Crash in LogBufferIterator::next

[James Peach <jp...@apache.org>]

On May 21, 2014, at 9:50 AM, briang@apache.org wrote:

> Repository: trafficserver
> Updated Branches:
>  refs/heads/master 2ba2baaa6 -> 374355c01
> 
> 
> [TS-2822] Crash in LogBufferIterator::next
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/7c314c5c
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/7c314c5c
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/7c314c5c
> 
> Branch: refs/heads/master
> Commit: 7c314c5c9bfa347fd583500698c2c586f668f7ea
> Parents: 2ba2baa
> Author: Brian Geffon <br...@apache.org>
> Authored: Wed May 21 09:49:13 2014 -0700
> Committer: Brian Geffon <br...@apache.org>
> Committed: Wed May 21 09:49:13 2014 -0700
> 
> ----------------------------------------------------------------------
> proxy/logstats.cc | 37 +++++++++++++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 6 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/7c314c5c/proxy/logstats.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/logstats.cc b/proxy/logstats.cc
> index 15aeaa2..c6386a6 100644
> --- a/proxy/logstats.cc
> +++ b/proxy/logstats.cc
> @@ -1672,6 +1672,7 @@ parse_log_buff(LogBufferHeader * buf_header, bool summary = false)
> int
> process_file(int in_fd, off_t offset, unsigned max_age)
> {
> +  ink_release_assert(in_fd < FD_SETSIZE); // If you're exceeding this then use CLOSEONEXEC with your opens before FORK/EXEC
>   char buffer[MAX_LOGBUFFER_SIZE];
>   int nread, buffer_bytes;
> 
> @@ -1730,7 +1731,7 @@ process_file(int in_fd, off_t offset, unsigned max_age)
>     unsigned second_read_size = sizeof(LogBufferHeader) - first_read_size;
>     nread = read(in_fd, &buffer[first_read_size], second_read_size);
>     if (!nread || EOF == nread) {
> -      Debug("logstats", "Second read of header failed (attemped %d bytes at offset %d, got nothing).", second_read_size, first_read_size);
> +      Debug("logstats", "Second read of header failed (attemped %d bytes at offset %d, got nothing), errno=%d.", second_read_size, first_read_size, errno);
>       return 1;
>     }
> 
> @@ -1746,11 +1747,35 @@ process_file(int in_fd, off_t offset, unsigned max_age)
>       return 1;
>     }
> 
> -    nread = read(in_fd, &buffer[sizeof(LogBufferHeader)], buffer_bytes);
> -    if (!nread || EOF == nread) {
> -      Debug("logstats", "Failed to read buffer payload [%d bytes]", buffer_bytes);
> -      return 1;
> -    }
> +    const int MAX_READ_TRIES = 5;
> +    int total_read = 0;
> +    int read_tries_remaining = MAX_READ_TRIES; // since the data will be old anyway, let's only try a few times.
> +    nread = 0;
> +    do {
> +      nread = read(in_fd, &buffer[sizeof(LogBufferHeader) + total_read], buffer_bytes - total_read);
> +      if (EOF == nread || !nread) { // just bail on error
> +        Debug("logstats", "Read failed while reading log buffer, wanted %d bytes, nread=%d, total_read=%d, errno=%d, tries_remaining=%d", buffer_bytes - total_read, nread, total_read,
> +            errno, read_tries_remaining);
> +        return 1;
> +      } else {
> +        total_read += nread;
> +      }
> +
> +      if (total_read < buffer_bytes) {
> +        if (--read_tries_remaining <= 0) {
> +          Debug("logstats_failed_retries", "Unable to read after %d tries, total_read=%d, buffer_bytes=%d", MAX_READ_TRIES, total_read, buffer_bytes);
> +          return 1;
> +        }
> +        // let's wait until we get more data on this file descriptor
> +        Debug("logstats_partial_read", "Failed to read buffer payload [%d bytes], total_read=%d, buffer_bytes=%d, tries_remaining=%d",
> +            buffer_bytes - total_read, total_read, buffer_bytes, read_tries_remaining);
> +        fd_set fds;
> +        struct timeval tv = {0, 50*1000}; // wait only up to 50ms
> +        FD_ZERO(&fds);
> +        FD_SET(in_fd, &fds);
> +        select(in_fd + 1, &fds, NULL, NULL, &tv);  // we only need to select up to in_fd + 1.

Can in_fd ever be a socket?


> +      }
> +    } while (total_read < buffer_bytes);
> 
>     // Possibly skip too old entries (the entire buffer is skipped)
>     if (header->high_timestamp >= max_age) {
>