Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 69c5c4c4 authored by JP Abgrall's avatar JP Abgrall
Browse files

Fix adb hang when subprocess dies early. 

* Handling of the subprocess and its FD.
  This fixes http://b/3400254

 "Many bugreports getting hung at the end in monkey"
  -  Start up a service thread that waits on the subprocess to terminate,
then closes the FD associated with it.
  -  Have the event handler select() with a timeout so that it can
  detect the closed FD. Select() with no timeout does not return when an FD is closed.
  - Have the event handler force a read on the closed FD to trigger the close sequence.
  - Migrate the "shell:blabla" handling to "#if !ADB_HOST" sections.
* Fix the race around OOM adjusting.
  - Do it in the child before exec() instead of the in the parent as the
   child could already have started or not (no /proc/pid/... yet).
* Allow for multi-threaded D() invocations to not clobber each other.
  - Allow locks across object files.
  - Add lock within D()
* Add some missing close(fd) calls
  - Match similar existing practices near dup2()
* Add extra D() invocations related to FD handling.
* Warn about using debugging as stderr/stdout is used for protocol.

Change-Id: Ie5c4a5e6bfbe3f22201adf5f9a205d32e069bf9d
Signed-off-by: default avatarJP Abgrall <jpa@google.com>
parent 53df0b63

adb/adb.c

+13 −9
Original line number Diff line number Diff line
@@ -36,6 +36,9 @@ 
#include "usb_vendors.h"

#endif



#if ADB_TRACE

ADB_MUTEX_DEFINE( D_lock );

#endif



int HOST = 0;
@@ -591,14 +594,6 @@ nomem: 
    return 0;

}



#ifdef HAVE_FORKEXEC

static void sigchld_handler(int n)

{

    int status;

    while(waitpid(-1, &status, WNOHANG) > 0) ;

}

#endif



#ifdef HAVE_WIN32_PROC

static BOOL WINAPI ctrlc_handler(DWORD type)

{
@@ -641,6 +636,7 @@ void start_logging(void) 


    fd = unix_open("/dev/null", O_RDONLY);

    dup2(fd, 0);

    adb_close(fd);



    fd = unix_open("/tmp/adb.log", O_WRONLY | O_CREAT | O_APPEND, 0640);

    if(fd < 0) {
@@ -648,6 +644,8 @@ void start_logging(void) 
    }

    dup2(fd, 1);

    dup2(fd, 2);

    adb_close(fd);



    fprintf(stderr,"--- adb starting (pid %d) ---\n", getpid());

#endif

}
@@ -792,6 +790,7 @@ int launch_server(int server_port) 
        // we use stderr instead of stdout due to stdout's buffering behavior.

        adb_close(fd[0]);

        dup2(fd[1], STDERR_FILENO);

        dup2(fd[1], STDOUT_FILENO);

        adb_close(fd[1]);



        // child process
@@ -848,7 +847,7 @@ int adb_main(int is_daemon, int server_port) 
#ifdef HAVE_WIN32_PROC

    SetConsoleCtrlHandler( ctrlc_handler, TRUE );

#elif defined(HAVE_FORKEXEC)

    signal(SIGCHLD, sigchld_handler);

    // Let the service subproc creator handle its children.

    signal(SIGPIPE, SIG_IGN);

#endif
@@ -957,7 +956,9 @@ int adb_main(int is_daemon, int server_port) 
        // listen on default port

        local_init(DEFAULT_ADB_LOCAL_TRANSPORT_PORT);

    }

    D("adb_main(): pre init_jdwp()\n");

    init_jdwp();

    D("adb_main(): post init_jdwp()\n");

#endif



    if (is_daemon)
@@ -971,6 +972,7 @@ int adb_main(int is_daemon, int server_port) 
#endif

        start_logging();

    }

    D("Event loop starting\n");



    fdevent_loop();
@@ -1271,6 +1273,7 @@ int main(int argc, char **argv) 
#if ADB_HOST

    adb_trace_init();

    adb_sysdeps_init();

    D("Handling commandline()\n");

    return adb_commandline(argc - 1, argv + 1);

#else

    if((argc > 1) && (!strcmp(argv[1],"recovery"))) {
@@ -1279,6 +1282,7 @@ int main(int argc, char **argv) 
    }



    start_device_log();

    D("Handling main()\n");

    return adb_main(0, DEFAULT_ADB_PORT);

#endif

}

adb/adb.h

+34 −4
Original line number Diff line number Diff line
@@ -344,8 +344,22 @@ typedef enum { 


#if ADB_TRACE



  int     adb_trace_mask;

// TODO(jpa): remove this after fixing fprintf() multithreading issue {

// Can't include sysdeps.h, because framebuffer_service.c wants a different

// close() than what sysdeps overrides when linking adbd.



// Assume Linux.

#ifndef _ADB_SYSDEPS_H

#include <pthread.h>

typedef  pthread_mutex_t          adb_mutex_t;

#define  ADB_MUTEX(x)   extern adb_mutex_t  x;

#include "mutex_list.h"

#endif



// }



  extern int     adb_trace_mask;

  extern unsigned char    adb_trace_output_count;

  void    adb_trace_init(void);



#  define ADB_TRACING  ((adb_trace_mask & (1 << TRACE_TAG)) != 0)
@@ -353,11 +367,27 @@ typedef enum { 
  /* you must define TRACE_TAG before using this macro */

#  define  D(...)                                      \

        do {                                           \

            if (ADB_TRACING)                           \

            if (ADB_TRACING) {                         \

                adb_mutex_lock(&D_lock);               \

                fprintf(stderr, "%s::%s():",           \

                    __FILE__, __FUNCTION__);           \

                fprintf(stderr, __VA_ARGS__ );         \

                fflush(stderr);                        \

                adb_mutex_unlock(&D_lock);             \

           }                                           \

        } while (0)

#  define  DR(...)                                     \

        do {                                           \

            if (ADB_TRACING) {                         \

                adb_mutex_lock(&D_lock);               \

                fprintf(stderr, __VA_ARGS__ );         \

                fflush(stderr);                        \

                adb_mutex_unlock(&D_lock);             \

           }                                           \

        } while (0)

#else

#  define  D(...)          ((void)0)

#  define  DR(...)         ((void)0)

#  define  ADB_TRACING     0

#endif

adb/adb_client.c

+3 −0
Original line number Diff line number Diff line
@@ -202,6 +202,7 @@ int _adb_connect(const char *service) 
        return -1;

    }



    D("_adb_connect: return fd %d\n", fd);

    return fd;

}
@@ -210,6 +211,7 @@ int adb_connect(const char *service) 
    // first query the adb server's version

    int fd = _adb_connect("host:version");



    D("adb_connect: service %s\n", service);

    if(fd == -2) {

        fprintf(stdout,"* daemon not running. starting it now on port %d *\n",

                __adb_server_port);
@@ -266,6 +268,7 @@ int adb_connect(const char *service) 
    if(fd == -2) {

        fprintf(stderr,"** daemon still not running");

    }

    D("adb_connect: return fd %d\n", fd);



    return fd;

error:

adb/commandline.c

+9 −0
Original line number Diff line number Diff line
@@ -218,7 +218,9 @@ static void read_and_dump(int fd) 
    int len;



    while(fd >= 0) {

        D("read_and_dump(): pre adb_read(fd=%d)\n", fd);

        len = adb_read(fd, buf, 4096);

        D("read_and_dump(): post adb_read(fd=%d): len=%d\n", fd, len);

        if(len == 0) {

            break;

        }
@@ -246,7 +248,9 @@ static void *stdin_read_thread(void *x) 


    for(;;) {

        /* fdi is really the client's stdin, so use read, not adb_read here */

        D("stdin_read_thread(): pre unix_read(fdi=%d,...)\n", fdi);

        r = unix_read(fdi, buf, 1024);

        D("stdin_read_thread(): post unix_read(fdi=%d,...)\n", fdi);

        if(r == 0) break;

        if(r < 0) {

            if(errno == EINTR) continue;
@@ -853,6 +857,7 @@ top: 
        }



        if(argc < 2) {

            D("starting interactive shell\n");

            r = interactive_shell();

            if (h) {

                printf("\x1b[0m");
@@ -877,9 +882,12 @@ top: 
        }



        for(;;) {

            D("interactive shell loop. buff=%s\n", buf);

            fd = adb_connect(buf);

            if(fd >= 0) {

                D("about to read_and_dump(fd=%d)\n", fd);

                read_and_dump(fd);

                D("read_and_dump() done.\n");

                adb_close(fd);

                r = 0;

            } else {
@@ -896,6 +904,7 @@ top: 
                    printf("\x1b[0m");

                    fflush(stdout);

                }

                D("interactive shell loop. return r=%d\n", r);

                return r;

            }

        }

adb/fdevent.c

+116 −27
Original line number Diff line number Diff line
@@ -28,9 +28,12 @@ 


#include "fdevent.h"



#define TRACE(x...) fprintf(stderr,x)



#define DEBUG 0

/* !!! Do not enable DEBUG for the adb that will run as the server:

** both stdout and stderr are used to communicate between the client

** and server. Any extra output will cause failures.

*/

#define DEBUG 0   /* non-0 will break adb server */



static void fatal(const char *fn, const char *fmt, ...)

{
@@ -45,6 +48,11 @@ static void fatal(const char *fn, const char *fmt, ...) 
#define FATAL(x...) fatal(__FUNCTION__, x)



#if DEBUG

#define D(...) \

    do { \

        fprintf(stderr, "%s::%s():", __FILE__, __FUNCTION__);  \

        fprintf(stderr, __VA_ARGS__);                          \

    } while(0)

static void dump_fde(fdevent *fde, const char *info)

{

    fprintf(stderr,"FDE #%03d %c%c%c %s\n", fde->fd,
@@ -54,6 +62,7 @@ static void dump_fde(fdevent *fde, const char *info) 
            info);

}

#else

#define D(...) ((void)0)

#define dump_fde(fde, info) do { } while(0)

#endif
@@ -273,39 +282,121 @@ static void fdevent_update(fdevent *fde, unsigned events) 
    fde->state = (fde->state & FDE_STATEMASK) | events;

}



#if DEBUG

static void dump_all_fds(const char *extra_msg)

{

int i;

    fdevent *fde;

    // per fd: 4 digits (but really: log10(FD_SETSIZE)), 1 staus, 1 blank

    char msg_buff[FD_SETSIZE*6 + 1], *pb=msg_buff;

    size_t max_chars = FD_SETSIZE * 6 + 1;

    int printed_out;

#define SAFE_SPRINTF(...)                                                    \

    do {                                                                     \

        printed_out = snprintf(pb, max_chars, __VA_ARGS__);                  \

        if (printed_out <= 0) {                                              \

            D("... snprintf failed.\n");                                     \

            return;                                                          \

        }                                                                    \

        if (max_chars < (unsigned int)printed_out) {                         \

            D("... snprintf out of space.\n");                               \

            return;                                                          \

        }                                                                    \

        pb += printed_out;                                                   \

        max_chars -= printed_out;                                            \

    } while(0)



    for(i = 0; i < select_n; i++) {

        fde = fd_table[i];

        SAFE_SPRINTF("%d", i);

        if(fde == 0) {

            SAFE_SPRINTF("? ");

            continue;

        }

        if(fcntl(i, F_GETFL, NULL) < 0) {

            SAFE_SPRINTF("b");

        }

        SAFE_SPRINTF(" ");

    }

    D("%s fd_table[]->fd = {%s}\n", extra_msg, msg_buff);

}

#endif



/* Looks at fd_table[] for bad FDs and sets bit in fds.

** Returns the number of bad FDs.

*/

static int fdevent_fd_check(fd_set *fds)

{

    int i, n = 0;

    fdevent *fde;



    for(i = 0; i < select_n; i++) {

        fde = fd_table[i];

        if(fde == 0) continue;

        if(fcntl(i, F_GETFL, NULL) < 0) {

            FD_SET(i, fds);

            n++;

            fde->state |= FDE_DONT_CLOSE;



        }

    }

    return n;

}



static void fdevent_process()

{

    int i, n;

    fdevent *fde;

    unsigned events;

    fd_set rfd, wfd, efd;

    struct timeval tv = { /*tv_sec=*/5, /*tv_usec=*/0 };



    memcpy(&rfd, &read_fds, sizeof(fd_set));

    memcpy(&wfd, &write_fds, sizeof(fd_set));

    memcpy(&efd, &error_fds, sizeof(fd_set));



    n = select(select_n, &rfd, &wfd, &efd, 0);

    n = select(select_n, &rfd, &wfd, &efd, &tv);



    D("select() returned n=%d, errno=%d\n", n, n<0?errno:0);



#if DEBUG

    dump_all_fds("post select()");

#endif



    if(n < 0) {

        if(errno == EINTR) return;

        perror("select");

        switch(errno) {

        case EINTR: return;

        case EBADF:

            // Can't trust the FD sets after an error.

            FD_ZERO(&wfd);

            FD_ZERO(&efd);

            FD_ZERO(&rfd);

            break;

        default:

            D("Unexpected select() error=%d\n", errno);

            return;

        }

    }

    if(n <= 0) {

        // We fake a read, as the rest of the code assumes

        // that errors will be detected at that point.

        n = fdevent_fd_check(&rfd);

    }



    for(i = 0; (i < select_n) && (n > 0); i++) {

        events = 0;

        if(FD_ISSET(i, &rfd)) events |= FDE_READ;

        if(FD_ISSET(i, &wfd)) events |= FDE_WRITE;

        if(FD_ISSET(i, &efd)) events |= FDE_ERROR;

        if(FD_ISSET(i, &rfd)) { events |= FDE_READ; n--; }

        if(FD_ISSET(i, &wfd)) { events |= FDE_WRITE; n--; }

        if(FD_ISSET(i, &efd)) { events |= FDE_ERROR; n--; }



        if(events) {

            n--;



            fde = fd_table[i];

            if(fde == 0) FATAL("missing fde for fd %d\n", i);



            fde->events |= events;



            D("got events fde->fd=%d events=%04x, state=%04x\n",

                fde->fd, fde->events, fde->state);

            if(fde->state & FDE_PENDING) continue;

            fde->state |= FDE_PENDING;

            fdevent_plist_enqueue(fde);
@@ -350,7 +441,7 @@ static void fdevent_unregister(fdevent *fde) 
    }



    if(fd_table[fde->fd] != fde) {

        FATAL("fd_table out of sync");

        FATAL("fd_table out of sync [%d]\n", fde->fd);

    }



    fd_table[fde->fd] = 0;
@@ -489,9 +580,8 @@ void fdevent_loop() 
    fdevent *fde;



    for(;;) {

#if DEBUG

        fprintf(stderr,"--- ---- waiting for events\n");

#endif

        D("--- ---- waiting for events\n");



        fdevent_process();



        while((fde = fdevent_plist_dequeue())) {
@@ -503,4 +593,3 @@ void fdevent_loop() 
        }

    }

}