From c3211557b3a7f1021878399a1e2574e697e92c24 Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Fri, 24 Jul 2015 15:38:19 -0700 Subject: [PATCH] adb: win32: file descriptor reliability improvements When repeatedly opening and closing a file descriptor, the sequence of fds returned was: 100...227,100,100,100,100,100... Basically, the first wave was constantly increasing fds, but after the entire fd table was traversed once, the alloc algorithm would switch to returning the first free fd. This is sub-optimal for reliability because use-after-free bugs would be more likely to be hit because right after a close, the same fd would be given out next. This change makes the alloc algorithm use a persistent clock hand that walks forward through the fd table (wrapping around if necessary), searching for a free fd. This change adds locking for fd closing: - This prevents multiple concurrent closes of the same fd. - There was a race between alloc and close that wasn't guaranteed to be correct: close would set f->clazz to NULL last, but without any preceding memory barrier/fence, then the alloc thread would check for NULL. It probably worked out ok in practice, but it is probably best to fix this up with a lock (as in this change) or a memory barrier/fence (but this code isn't about performance, so why go with a complicated barrier/fence?) Also in this change: - Use errno = EMFILE for the out of fds case. - Clear FH->name Change-Id: Ic11d2a1a9d53996edfc1ca13566a2f46de4a4316 Signed-off-by: Spencer Low --- adb/sysdeps_win32.cpp | 47 +++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index 9fdc24c8b..093e90220 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -201,7 +201,7 @@ typedef struct FHRec_ static adb_mutex_t _win32_lock; static FHRec _win32_fhs[ WIN32_MAX_FHS ]; -static int _win32_fh_count; +static int _win32_fh_next; // where to start search for free FHRec static FH _fh_from_int( int fd, const char* func ) @@ -210,7 +210,7 @@ _fh_from_int( int fd, const char* func ) fd -= WIN32_FH_BASE; - if (fd < 0 || fd >= _win32_fh_count) { + if (fd < 0 || fd >= WIN32_MAX_FHS) { D( "_fh_from_int: invalid fd %d passed to %s\n", fd + WIN32_FH_BASE, func ); errno = EBADF; @@ -242,28 +242,32 @@ _fh_to_int( FH f ) static FH _fh_alloc( FHClass clazz ) { - int nn; FH f = NULL; adb_mutex_lock( &_win32_lock ); - if (_win32_fh_count < WIN32_MAX_FHS) { - f = &_win32_fhs[ _win32_fh_count++ ]; - goto Exit; - } - - for (nn = 0; nn < WIN32_MAX_FHS; nn++) { - if ( _win32_fhs[nn].clazz == NULL) { - f = &_win32_fhs[nn]; + // Search entire array, starting from _win32_fh_next. + for (int nn = 0; nn < WIN32_MAX_FHS; nn++) { + // Keep incrementing _win32_fh_next to avoid giving out an index that + // was recently closed, to try to avoid use-after-free. + const int index = _win32_fh_next++; + // Handle wrap-around of _win32_fh_next. + if (_win32_fh_next == WIN32_MAX_FHS) { + _win32_fh_next = 0; + } + if (_win32_fhs[index].clazz == NULL) { + f = &_win32_fhs[index]; goto Exit; } } D( "_fh_alloc: no more free file descriptors\n" ); + errno = EMFILE; // Too many open files Exit: if (f) { - f->clazz = clazz; - f->used = 1; - f->eof = 0; + f->clazz = clazz; + f->used = 1; + f->eof = 0; + f->name[0] = '\0'; clazz->_fh_init(f); } adb_mutex_unlock( &_win32_lock ); @@ -274,12 +278,17 @@ Exit: static int _fh_close( FH f ) { - if ( f->used ) { + // Use lock so that closing only happens once and so that _fh_alloc can't + // allocate a FH that we're in the middle of closing. + adb_mutex_lock(&_win32_lock); + if (f->used) { f->clazz->_fh_close( f ); - f->used = 0; - f->eof = 0; - f->clazz = NULL; + f->name[0] = '\0'; + f->eof = 0; + f->used = 0; + f->clazz = NULL; } + adb_mutex_unlock(&_win32_lock); return 0; } @@ -402,7 +411,6 @@ int adb_open(const char* path, int options) f = _fh_alloc( &_fh_file_class ); if ( !f ) { - errno = ENOMEM; return -1; } @@ -443,7 +451,6 @@ int adb_creat(const char* path, int mode) f = _fh_alloc( &_fh_file_class ); if ( !f ) { - errno = ENOMEM; return -1; }