adb: fix adb_close() vs. unix_close() usage
Document the differences between adb_*() and unix_*() in the function
prototypes in sysdeps.h. See the file for the details (CR/LF
translation, well-known file descriptors, etc.).
Fix adb_read(), adb_write(), and adb_close() calls that should really be
unix_read(), unix_write(), and unix_close(). Note that this should have
no impact on unix because on unix, unix_read/unix_write/unix_close are
macros that map to adb_read/adb_write/adb_close.
Improve sysdeps_win32.cpp file descriptor diagnostic logging to output
the name of the function that was passed a bad file descriptor.
Change-Id: I0a1d9c28772656c80bcc303ef8b61fccf4cd637c
Signed-off-by: Spencer Low <CompareAndSwap@gmail.com>
diff --git a/adb/adb.cpp b/adb/adb.cpp
index 2c959a4..868bfbc 100644
--- a/adb/adb.cpp
+++ b/adb/adb.cpp
@@ -115,7 +115,7 @@
dup2(fd, STDOUT_FILENO);
dup2(fd, STDERR_FILENO);
fprintf(stderr, "--- adb starting (pid %d) ---\n", getpid());
- adb_close(fd);
+ unix_close(fd);
}
#endif
diff --git a/adb/adb_auth_host.cpp b/adb/adb_auth_host.cpp
index 61a3777..e878f8b 100644
--- a/adb/adb_auth_host.cpp
+++ b/adb/adb_auth_host.cpp
@@ -420,6 +420,9 @@
strcat(path, ".pub");
// TODO(danalbert): ReadFileToString
+ // Note that on Windows, load_file() does not do CR/LF translation, but
+ // ReadFileToString() uses the C Runtime which uses CR/LF translation by
+ // default (by is overridable with _setmode()).
unsigned size;
char* file_data = reinterpret_cast<char*>(load_file(path, &size));
if (file_data == nullptr) {
diff --git a/adb/daemon/main.cpp b/adb/daemon/main.cpp
index c0612cd..78ab3f6 100644
--- a/adb/daemon/main.cpp
+++ b/adb/daemon/main.cpp
@@ -226,7 +226,7 @@
return;
}
dup2(fd, STDIN_FILENO);
- adb_close(fd);
+ unix_close(fd);
}
int main(int argc, char** argv) {
diff --git a/adb/remount_service.cpp b/adb/remount_service.cpp
index 7a3b89a..3a4d2da 100644
--- a/adb/remount_service.cpp
+++ b/adb/remount_service.cpp
@@ -58,7 +58,7 @@
int OFF = 0;
bool result = (ioctl(fd, BLKROSET, &OFF) != -1);
- adb_close(fd);
+ unix_close(fd);
return result;
}
diff --git a/adb/services.cpp b/adb/services.cpp
index a9edbe5..2fd75ab 100644
--- a/adb/services.cpp
+++ b/adb/services.cpp
@@ -252,7 +252,7 @@
*pid = forkpty(&ptm, pts_name, nullptr, nullptr);
if (*pid == -1) {
printf("- fork failed: %s -\n", strerror(errno));
- adb_close(ptm);
+ unix_close(ptm);
return -1;
}
@@ -263,7 +263,7 @@
if (pts == -1) {
fprintf(stderr, "child failed to open pseudo-term slave %s: %s\n",
pts_name, strerror(errno));
- adb_close(ptm);
+ unix_close(ptm);
exit(-1);
}
@@ -272,16 +272,16 @@
termios tattr;
if (tcgetattr(pts, &tattr) == -1) {
fprintf(stderr, "tcgetattr failed: %s\n", strerror(errno));
- adb_close(pts);
- adb_close(ptm);
+ unix_close(pts);
+ unix_close(ptm);
exit(-1);
}
cfmakeraw(&tattr);
if (tcsetattr(pts, TCSADRAIN, &tattr) == -1) {
fprintf(stderr, "tcsetattr failed: %s\n", strerror(errno));
- adb_close(pts);
- adb_close(ptm);
+ unix_close(pts);
+ unix_close(ptm);
exit(-1);
}
}
@@ -290,8 +290,8 @@
dup2(pts, STDOUT_FILENO);
dup2(pts, STDERR_FILENO);
- adb_close(pts);
- adb_close(ptm);
+ unix_close(pts);
+ unix_close(ptm);
execl(cmd, cmd, arg0, arg1, nullptr);
fprintf(stderr, "- exec '%s' failed: %s (%d) -\n",
diff --git a/adb/sysdeps.h b/adb/sysdeps.h
index 0aa1570..b7c16d9 100644
--- a/adb/sysdeps.h
+++ b/adb/sysdeps.h
@@ -125,6 +125,7 @@
#undef mkdir
#define mkdir ___xxx_mkdir
+// See the comments for the !defined(_WIN32) versions of adb_*().
extern int adb_open(const char* path, int options);
extern int adb_creat(const char* path, int mode);
extern int adb_read(int fd, void* buf, int len);
@@ -133,6 +134,7 @@
extern int adb_shutdown(int fd);
extern int adb_close(int fd);
+// See the comments for the !defined(_WIN32) version of unix_close().
static __inline__ int unix_close(int fd)
{
return close(fd);
@@ -140,11 +142,13 @@
#undef close
#define close ____xxx_close
+// See the comments for the !defined(_WIN32) version of unix_read().
extern int unix_read(int fd, void* buf, size_t len);
#undef read
#define read ___xxx_read
+// See the comments for the !defined(_WIN32) version of unix_write().
static __inline__ int unix_write(int fd, const void* buf, size_t len)
{
return write(fd, buf, len);
@@ -152,11 +156,13 @@
#undef write
#define write ___xxx_write
+// See the comments for the !defined(_WIN32) version of adb_open_mode().
static __inline__ int adb_open_mode(const char* path, int options, int mode)
{
return adb_open(path, options);
}
+// See the comments for the !defined(_WIN32) version of unix_open().
static __inline__ int unix_open(const char* path, int options,...)
{
if ((options & O_CREAT) == 0)
@@ -314,6 +320,15 @@
fcntl( fd, F_SETFD, FD_CLOEXEC );
}
+// Open a file and return a file descriptor that may be used with unix_read(),
+// unix_write(), unix_close(), but not adb_read(), adb_write(), adb_close().
+//
+// On Unix, this is based on open(), so the file descriptor is a real OS file
+// descriptor, but the Windows implementation (in sysdeps_win32.cpp) returns a
+// file descriptor that can only be used with C Runtime APIs (which are wrapped
+// by unix_read(), unix_write(), unix_close()). Also, the C Runtime has
+// configurable CR/LF translation which defaults to text mode, but is settable
+// with _setmode().
static __inline__ int unix_open(const char* path, int options,...)
{
if ((options & O_CREAT) == 0)
@@ -331,12 +346,21 @@
}
}
+// Similar to the two-argument adb_open(), but takes a mode parameter for file
+// creation. See adb_open() for more info.
static __inline__ int adb_open_mode( const char* pathname, int options, int mode )
{
return TEMP_FAILURE_RETRY( open( pathname, options, mode ) );
}
+// Open a file and return a file descriptor that may be used with adb_read(),
+// adb_write(), adb_close(), but not unix_read(), unix_write(), unix_close().
+//
+// On Unix, this is based on open(), but the Windows implementation (in
+// sysdeps_win32.cpp) uses Windows native file I/O and bypasses the C Runtime
+// and its CR/LF translation. The returned file descriptor should be used with
+// adb_read(), adb_write(), adb_close(), etc.
static __inline__ int adb_open( const char* pathname, int options )
{
int fd = TEMP_FAILURE_RETRY( open( pathname, options ) );
@@ -355,6 +379,9 @@
#undef shutdown
#define shutdown ____xxx_shutdown
+// Closes a file descriptor that came from adb_open() or adb_open_mode(), but
+// not designed to take a file descriptor from unix_open(). See the comments
+// for adb_open() for more info.
static __inline__ int adb_close(int fd)
{
return close(fd);
@@ -419,6 +446,14 @@
#undef accept
#define accept ___xxx_accept
+// Operate on a file descriptor returned from unix_open() or a well-known file
+// descriptor such as STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO.
+//
+// On Unix, unix_read(), unix_write(), unix_close() map to adb_read(),
+// adb_write(), adb_close() (which all map to Unix system calls), but the
+// Windows implementations (in the ifdef above and in sysdeps_win32.cpp) call
+// into the C Runtime and its configurable CR/LF translation (which is settable
+// via _setmode()).
#define unix_read adb_read
#define unix_write adb_write
#define unix_close adb_close
diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp
index a21272f..50c99f1 100644
--- a/adb/sysdeps_win32.cpp
+++ b/adb/sysdeps_win32.cpp
@@ -172,14 +172,15 @@
static int _win32_fh_count;
static FH
-_fh_from_int( int fd )
+_fh_from_int( int fd, const char* func )
{
FH f;
fd -= WIN32_FH_BASE;
if (fd < 0 || fd >= _win32_fh_count) {
- D( "_fh_from_int: invalid fd %d\n", fd + WIN32_FH_BASE );
+ D( "_fh_from_int: invalid fd %d passed to %s\n", fd + WIN32_FH_BASE,
+ func );
errno = EBADF;
return NULL;
}
@@ -187,7 +188,8 @@
f = &_win32_fhs[fd];
if (f->used == 0) {
- D( "_fh_from_int: invalid fd %d\n", fd + WIN32_FH_BASE );
+ D( "_fh_from_int: invalid fd %d passed to %s\n", fd + WIN32_FH_BASE,
+ func );
errno = EBADF;
return NULL;
}
@@ -427,7 +429,7 @@
int adb_read(int fd, void* buf, int len)
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
if (f == NULL) {
return -1;
@@ -439,7 +441,7 @@
int adb_write(int fd, const void* buf, int len)
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
if (f == NULL) {
return -1;
@@ -451,7 +453,7 @@
int adb_lseek(int fd, int pos, int where)
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
if (!f) {
return -1;
@@ -463,7 +465,7 @@
int adb_shutdown(int fd)
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
if (!f || f->clazz != &_fh_socket_class) {
D("adb_shutdown: invalid fd %d\n", fd);
@@ -478,7 +480,7 @@
int adb_close(int fd)
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
if (!f) {
return -1;
@@ -763,7 +765,7 @@
#undef accept
int adb_socket_accept(int serverfd, struct sockaddr* addr, socklen_t *addrlen)
{
- FH serverfh = _fh_from_int(serverfd);
+ FH serverfh = _fh_from_int(serverfd, __func__);
FH fh;
if ( !serverfh || serverfh->clazz != &_fh_socket_class ) {
@@ -792,7 +794,7 @@
int adb_setsockopt( int fd, int level, int optname, const void* optval, socklen_t optlen )
{
- FH fh = _fh_from_int(fd);
+ FH fh = _fh_from_int(fd, __func__);
if ( !fh || fh->clazz != &_fh_socket_class ) {
D("adb_setsockopt: invalid fd %d\n", fd);
@@ -1386,7 +1388,7 @@
static void
event_looper_hook( EventLooper looper, int fd, int events )
{
- FH f = _fh_from_int(fd);
+ FH f = _fh_from_int(fd, __func__);
EventHook *pnode;
EventHook node;
@@ -1418,7 +1420,7 @@
static void
event_looper_unhook( EventLooper looper, int fd, int events )
{
- FH fh = _fh_from_int(fd);
+ FH fh = _fh_from_int(fd, __func__);
EventHook *pnode = event_looper_find_p( looper, fh );
EventHook node = *pnode;
@@ -3036,7 +3038,7 @@
}
}
-// Called by 'adb shell' command to read from stdin.
+// Called by 'adb shell' and 'adb exec-in' to read from stdin.
int unix_read(int fd, void* buf, size_t len) {
if ((fd == STDIN_FILENO) && (_console_handle != NULL)) {
// If it is a request to read from stdin, and stdin_raw_init() has been
@@ -3046,8 +3048,12 @@
return _console_read(_console_handle, buf, len);
} else {
// Just call into C Runtime which can read from pipes/files and which
- // can do LF/CR translation.
+ // can do LF/CR translation (which is overridable with _setmode()).
+ // Undefine the macro that is set in sysdeps.h which bans calls to
+ // plain read() in favor of unix_read() or adb_read().
+#pragma push_macro("read")
#undef read
return read(fd, buf, len);
+#pragma pop_macro("read")
}
}
diff --git a/adb/usb_linux.cpp b/adb/usb_linux.cpp
index 1e5d5c8..c6f712b 100644
--- a/adb/usb_linux.cpp
+++ b/adb/usb_linux.cpp
@@ -168,13 +168,13 @@
continue;
}
- desclength = adb_read(fd, devdesc, sizeof(devdesc));
+ desclength = unix_read(fd, devdesc, sizeof(devdesc));
bufend = bufptr + desclength;
// should have device and configuration descriptors, and atleast two endpoints
if (desclength < USB_DT_DEVICE_SIZE + USB_DT_CONFIG_SIZE) {
D("desclength %zu is too small\n", desclength);
- adb_close(fd);
+ unix_close(fd);
continue;
}
@@ -182,7 +182,7 @@
bufptr += USB_DT_DEVICE_SIZE;
if((device->bLength != USB_DT_DEVICE_SIZE) || (device->bDescriptorType != USB_DT_DEVICE)) {
- adb_close(fd);
+ unix_close(fd);
continue;
}
@@ -195,7 +195,7 @@
bufptr += USB_DT_CONFIG_SIZE;
if (config->bLength != USB_DT_CONFIG_SIZE || config->bDescriptorType != USB_DT_CONFIG) {
D("usb_config_descriptor not found\n");
- adb_close(fd);
+ unix_close(fd);
continue;
}
@@ -303,7 +303,7 @@
}
} // end of while
- adb_close(fd);
+ unix_close(fd);
} // end of devdir while
closedir(devdir);
} //end of busdir while
@@ -555,7 +555,7 @@
h->prev = 0;
h->next = 0;
- adb_close(h->desc);
+ unix_close(h->desc);
D("-- usb closed %p (fd = %d) --\n", h, h->desc);
adb_mutex_unlock(&usb_lock);
@@ -618,7 +618,7 @@
if (ioctl(usb->desc, USBDEVFS_CLAIMINTERFACE, &interface) != 0) {
D("[ usb ioctl(%d, USBDEVFS_CLAIMINTERFACE) failed: %s]\n",
usb->desc, strerror(errno));
- adb_close(usb->desc);
+ unix_close(usb->desc);
free(usb);
return;
}
diff --git a/adb/usb_linux_client.cpp b/adb/usb_linux_client.cpp
index 63bb1c7..d34c454 100644
--- a/adb/usb_linux_client.cpp
+++ b/adb/usb_linux_client.cpp
@@ -201,7 +201,7 @@
int n;
D("about to write (fd=%d, len=%d)\n", h->fd, len);
- n = adb_write(h->fd, data, len);
+ n = unix_write(h->fd, data, len);
if(n != len) {
D("ERROR: fd = %d, n = %d, errno = %d (%s)\n",
h->fd, n, errno, strerror(errno));
@@ -216,7 +216,7 @@
int n;
D("about to read (fd=%d, len=%d)\n", h->fd, len);
- n = adb_read(h->fd, data, len);
+ n = unix_read(h->fd, data, len);
if(n != len) {
D("ERROR: fd = %d, n = %d, errno = %d (%s)\n",
h->fd, n, errno, strerror(errno));
@@ -230,7 +230,7 @@
{
D("usb_kick\n");
adb_mutex_lock(&h->lock);
- adb_close(h->fd);
+ unix_close(h->fd);
h->fd = -1;
// notify usb_adb_open_thread that we are disconnected