TS-898: ensure cache span probe closes file descriptors Add a new helper class xfd (by analogy to xptr). This ensures that the opened file descriptor is always closed in every error path.
Coverity #1196462 Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/e503ce04 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/e503ce04 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/e503ce04 Branch: refs/heads/5.0.x Commit: e503ce048bb5fcab84b251ebcc31efa972d67710 Parents: 17579ee Author: James Peach <[email protected]> Authored: Sun Apr 6 11:25:11 2014 -0700 Committer: James Peach <[email protected]> Committed: Thu Apr 10 14:43:22 2014 -0700 ---------------------------------------------------------------------- CHANGES | 2 ++ iocore/cache/Store.cc | 27 +++++++++----------- iocore/eventsystem/I_SocketManager.h | 42 +++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e503ce04/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 3f113bc..2115ad8 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ -*- coding: utf-8 -*- Changes with Apache Traffic Server 5.0.0 + *) [TS-898] Ensure the cache span probe always closes file descriptors. + *) [TS-2706] Replace the tcp_info plugin config file with options. *) [TS-2691] Fix how we count redirect retries in the core and APIs. http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e503ce04/iocore/cache/Store.cc ---------------------------------------------------------------------- diff --git a/iocore/cache/Store.cc b/iocore/cache/Store.cc index 2e79958..46c346c 100644 --- a/iocore/cache/Store.cc +++ b/iocore/cache/Store.cc @@ -496,16 +496,15 @@ Span::init(char *an, int64_t size) return "error stat of file"; } - int fd = socketManager.open(n, O_RDONLY); - if (fd < 0) { - Warning("unable to open '%s': %d, %s", n, fd, strerror(errno)); + xfd fd(socketManager.open(n, O_RDONLY)); + if (!fd) { + Warning("unable to open '%s': %s", n, strerror(errno)); return "unable to open"; } struct statvfs fs; if ((ret = fstatvfs(fd, &fs)) < 0) { Warning("unable to statvfs '%s': %d %d, %s", n, ret, errno, strerror(errno)); - socketManager.close(fd); return "unable to statvfs"; } @@ -569,7 +568,6 @@ Span::init(char *an, int64_t size) Debug("cache_init", "Span::init - %s hw_sector_size = %d size = %" PRId64 ", blocks = %" PRId64 ", disk_id = %d, file_pathname = %d", pathname, hw_sector_size, size, blocks, disk_id, file_pathname); Lfail: - socketManager.close(fd); return err; } @@ -589,9 +587,9 @@ Span::init(char *filename, int64_t size) // is_mmapable_internal = true; - int fd = socketManager.open(filename, O_RDONLY); - if (fd < 0) { - Warning("unable to open '%s': %d, %s", filename, fd, strerror(errno)); + xfd fd(socketManager.open(filename, O_RDONLY)); + if (!fd) { + Warning("unable to open '%s': %s", filename, strerror(errno)); return "unable to open"; } @@ -657,7 +655,6 @@ Span::init(char *filename, int64_t size) Debug("cache_init", "Span::init - %s hw_sector_size = %d size = %" PRId64 ", blocks = %" PRId64 ", disk_id = %d, file_pathname = %d", filename, hw_sector_size, size, blocks, disk_id, file_pathname); Lfail: - socketManager.close(fd); return err; } #endif @@ -672,9 +669,10 @@ Lfail: const char * Span::init(char *filename, int64_t size) { - int devnum = 0, fd, arg = 0; + int devnum = 0, arg = 0; int ret = 0, is_disk = 0; u_int64_t heads, sectors, cylinders, adjusted_sec; + xfd fd; /* Fetch file type */ struct stat stat_buf; @@ -705,11 +703,12 @@ Span::init(char *filename, int64_t size) break; } - if ((fd = socketManager.open(filename, O_RDONLY)) < 0) { - Warning("unable to open '%s': %d, %s", filename, fd, strerror(errno)); + fd = socketManager.open(filename, O_RDONLY); + if (!fd) { + Warning("unable to open '%s': %s", filename, strerror(errno)); return "unable to open"; } - Debug("cache_init", "Span::init - socketManager.open(\"%s\", O_RDONLY) = %d", filename, fd); + Debug("cache_init", "Span::init - socketManager.open(\"%s\", O_RDONLY) = %d", filename, (int)fd); adjusted_sec = 1; #ifdef BLKPBSZGET @@ -822,8 +821,6 @@ Span::init(char *filename, int64_t size) disk_id = devnum; - socketManager.close(fd); - return NULL; } #endif http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e503ce04/iocore/eventsystem/I_SocketManager.h ---------------------------------------------------------------------- diff --git a/iocore/eventsystem/I_SocketManager.h b/iocore/eventsystem/I_SocketManager.h index d294a5c..6d3dc07 100644 --- a/iocore/eventsystem/I_SocketManager.h +++ b/iocore/eventsystem/I_SocketManager.h @@ -134,4 +134,46 @@ private: extern SocketManager socketManager; +struct xfd { + + xfd() : m_fd(-1) { + } + + explicit xfd(int _fd) : m_fd(_fd) { + } + + ~xfd() { + if (this->m_fd != -1) { + socketManager.close(this->m_fd); + } + } + + /// Auto convert to a raw file descriptor. + operator int() const { return m_fd; } + + /// Boolean operator. Returns true if we have a valid file descriptor. + operator bool() const { return m_fd != -1; } + + xfd& operator=(int fd) { + if (this->m_fd != -1) { + socketManager.close(this->m_fd); + } + + this->m_fd = fd; + return *this; + } + + int release() { + int tmp = this->m_fd; + this->m_fd = -1; + return tmp; + } + + private: + int m_fd; + + xfd(xfd const&); // disabled + xfd& operator=(xfd const&); // disabled +}; + #endif /*_SocketManager_h_*/
