From 60a0375ff99ae2d7608006ebda19d41bcf517b86 Mon Sep 17 00:00:00 2001 From: Vojtech Kral Date: Tue, 7 Aug 2018 11:28:39 +0200 Subject: [PATCH] Firmware updater: Fix a race condition avrdude: Handle OOM with configurable handler --- xs/src/avrdude/avrdude-slic3r.cpp | 95 ++++++++++++++++++++++------ xs/src/avrdude/avrdude-slic3r.hpp | 15 ++++- xs/src/avrdude/avrdude.h | 6 ++ xs/src/avrdude/avrpart.c | 34 ++++++---- xs/src/avrdude/buspirate.c | 7 +- xs/src/avrdude/butterfly.c | 7 +- xs/src/avrdude/lexer.c | 8 ++- xs/src/avrdude/main.c | 27 ++++++++ xs/src/avrdude/pgm.c | 7 +- xs/src/avrdude/ser_win32.c | 9 +-- xs/src/avrdude/stk500v2.c | 7 +- xs/src/avrdude/update.c | 25 +++++--- xs/src/avrdude/wiring.c | 7 +- xs/src/slic3r/GUI/FirmwareDialog.cpp | 6 +- 14 files changed, 193 insertions(+), 67 deletions(-) diff --git a/xs/src/avrdude/avrdude-slic3r.cpp b/xs/src/avrdude/avrdude-slic3r.cpp index 0577fe6d0..3037f5284 100644 --- a/xs/src/avrdude/avrdude-slic3r.cpp +++ b/xs/src/avrdude/avrdude-slic3r.cpp @@ -2,6 +2,10 @@ #include #include +#include +#include +#include +#include extern "C" { #include "ac_cfg.h" @@ -28,6 +32,11 @@ static void avrdude_progress_handler_closure(const char *task, unsigned progress (*progress_fn)(task, progress); } +static void avrdude_oom_handler(const char *context, void *user_p) +{ + throw std::bad_alloc(); +} + // Private @@ -47,16 +56,22 @@ struct AvrDude::priv priv(std::string &&sys_config) : sys_config(sys_config) {} + void set_handlers(); + void unset_handlers(); int run_one(const std::vector &args); int run(); + + struct HandlerGuard + { + priv &p; + + HandlerGuard(priv &p) : p(p) { p.set_handlers(); } + ~HandlerGuard() { p.unset_handlers(); } + }; }; -int AvrDude::priv::run_one(const std::vector &args) { - std::vector c_args {{ const_cast(PACKAGE_NAME) }}; - for (const auto &arg : args) { - c_args.push_back(const_cast(arg.data())); - } - +void AvrDude::priv::set_handlers() +{ if (message_fn) { ::avrdude_message_handler_set(avrdude_message_handler_closure, reinterpret_cast(&message_fn)); } else { @@ -69,10 +84,27 @@ int AvrDude::priv::run_one(const std::vector &args) { ::avrdude_progress_handler_set(nullptr, nullptr); } - const auto res = ::avrdude_main(static_cast(c_args.size()), c_args.data(), sys_config.c_str()); + ::avrdude_oom_handler_set(avrdude_oom_handler, nullptr); +} +void AvrDude::priv::unset_handlers() +{ ::avrdude_message_handler_set(nullptr, nullptr); ::avrdude_progress_handler_set(nullptr, nullptr); + ::avrdude_oom_handler_set(nullptr, nullptr); +} + + +int AvrDude::priv::run_one(const std::vector &args) { + std::vector c_args {{ const_cast(PACKAGE_NAME) }}; + for (const auto &arg : args) { + c_args.push_back(const_cast(arg.data())); + } + + HandlerGuard guard(*this); + + const auto res = ::avrdude_main(static_cast(c_args.size()), c_args.data(), sys_config.c_str()); + return res; } @@ -134,7 +166,7 @@ AvrDude& AvrDude::on_complete(CompleteFn fn) int AvrDude::run_sync() { - return p->run(); + return p ? p->run() : -1; } AvrDude::Ptr AvrDude::run() @@ -143,19 +175,46 @@ AvrDude::Ptr AvrDude::run() if (self->p) { auto avrdude_thread = std::thread([self]() { - bool cancel = false; - int res = -1; + try { + if (self->p->run_fn) { + self->p->run_fn(self); + } - if (self->p->run_fn) { - self->p->run_fn(); - } + if (! self->p->cancelled) { + self->p->exit_code = self->p->run(); + } - if (! self->p->cancelled) { - self->p->exit_code = self->p->run(); - } + if (self->p->complete_fn) { + self->p->complete_fn(); + } + } catch (const std::exception &ex) { + self->p->exit_code = EXIT_EXCEPTION; - if (self->p->complete_fn) { - self->p->complete_fn(); + static const char *msg = "An exception was thrown in the background thread:\n"; + + const char *what = ex.what(); + auto &message_fn = self->p->message_fn; + if (message_fn) { + message_fn(msg, sizeof(msg)); + message_fn(what, std::strlen(what)); + message_fn("\n", 1); + } + + if (self->p->complete_fn) { + self->p->complete_fn(); + } + } catch (...) { + self->p->exit_code = EXIT_EXCEPTION; + + static const char *msg = "An unkown exception was thrown in the background thread.\n"; + + if (self->p->message_fn) { + self->p->message_fn(msg, sizeof(msg)); + } + + if (self->p->complete_fn) { + self->p->complete_fn(); + } } }); diff --git a/xs/src/avrdude/avrdude-slic3r.hpp b/xs/src/avrdude/avrdude-slic3r.hpp index 86e097034..754e1e345 100644 --- a/xs/src/avrdude/avrdude-slic3r.hpp +++ b/xs/src/avrdude/avrdude-slic3r.hpp @@ -11,8 +11,13 @@ namespace Slic3r { class AvrDude { public: + enum { + EXIT_SUCCEESS = 0, + EXIT_EXCEPTION = -1000, + }; + typedef std::shared_ptr Ptr; - typedef std::function RunFn; + typedef std::function RunFn; typedef std::function MessageFn; typedef std::function ProgressFn; typedef std::function CompleteFn; @@ -49,10 +54,18 @@ public: // This has no effect when using run_sync(). AvrDude& on_complete(CompleteFn fn); + // Perform AvrDude invocation(s) synchronously on the current thread int run_sync(); + + // Perform AvrDude invocation(s) on a background thread. + // Current instance is moved into a shared_ptr which is returned (and also passed in on_run, if any). Ptr run(); + // Cancel current operation void cancel(); + + // If there is a background thread and it is joinable, join() it, + // that is, wait for it to finish. void join(); bool cancelled(); // Whether avrdude run was cancelled diff --git a/xs/src/avrdude/avrdude.h b/xs/src/avrdude/avrdude.h index 9f724433f..f4c92a75d 100644 --- a/xs/src/avrdude/avrdude.h +++ b/xs/src/avrdude/avrdude.h @@ -39,6 +39,12 @@ typedef void (*avrdude_progress_handler_t)(const char *task, unsigned progress, void avrdude_progress_handler_set(avrdude_progress_handler_t newhandler, void *user_p); void avrdude_progress_external(const char *task, unsigned progress); +// OOM handler +typedef void (*avrdude_oom_handler_t)(const char *context, void *user_p); +void avrdude_oom_handler_set(avrdude_oom_handler_t newhandler, void *user_p); +void avrdude_oom(const char *context); + + // Cancellation void avrdude_cancel(); diff --git a/xs/src/avrdude/avrpart.c b/xs/src/avrdude/avrpart.c index b04851ac1..d0bb951ee 100644 --- a/xs/src/avrdude/avrpart.c +++ b/xs/src/avrdude/avrpart.c @@ -36,8 +36,9 @@ OPCODE * avr_new_opcode(void) m = (OPCODE *)malloc(sizeof(*m)); if (m == NULL) { - avrdude_message(MSG_INFO, "avr_new_opcode(): out of memory\n"); - exit(1); + // avrdude_message(MSG_INFO, "avr_new_opcode(): out of memory\n"); + // exit(1); + avrdude_oom("avr_new_opcode(): out of memory\n"); } memset(m, 0, sizeof(*m)); @@ -56,8 +57,9 @@ static OPCODE * avr_dup_opcode(OPCODE * op) m = (OPCODE *)malloc(sizeof(*m)); if (m == NULL) { - avrdude_message(MSG_INFO, "avr_dup_opcode(): out of memory\n"); - exit(1); + // avrdude_message(MSG_INFO, "avr_dup_opcode(): out of memory\n"); + // exit(1); + avrdude_oom("avr_dup_opcode(): out of memory\n"); } memcpy(m, op, sizeof(*m)); @@ -249,8 +251,9 @@ AVRMEM * avr_new_memtype(void) m = (AVRMEM *)malloc(sizeof(*m)); if (m == NULL) { - avrdude_message(MSG_INFO, "avr_new_memtype(): out of memory\n"); - exit(1); + // avrdude_message(MSG_INFO, "avr_new_memtype(): out of memory\n"); + // exit(1); + avrdude_oom("avr_new_memtype(): out of memory\n"); } memset(m, 0, sizeof(*m)); @@ -300,9 +303,10 @@ AVRMEM * avr_dup_mem(AVRMEM * m) if (m->buf != NULL) { n->buf = (unsigned char *)malloc(n->size); if (n->buf == NULL) { - avrdude_message(MSG_INFO, "avr_dup_mem(): out of memory (memsize=%d)\n", - n->size); - exit(1); + // avrdude_message(MSG_INFO, "avr_dup_mem(): out of memory (memsize=%d)\n", + // n->size); + // exit(1); + avrdude_oom("avr_dup_mem(): out of memory"); } memcpy(n->buf, m->buf, n->size); } @@ -310,9 +314,10 @@ AVRMEM * avr_dup_mem(AVRMEM * m) if (m->tags != NULL) { n->tags = (unsigned char *)malloc(n->size); if (n->tags == NULL) { - avrdude_message(MSG_INFO, "avr_dup_mem(): out of memory (memsize=%d)\n", - n->size); - exit(1); + // avrdude_message(MSG_INFO, "avr_dup_mem(): out of memory (memsize=%d)\n", + // n->size); + // exit(1); + avrdude_oom("avr_dup_mem(): out of memory"); } memcpy(n->tags, m->tags, n->size); } @@ -441,8 +446,9 @@ AVRPART * avr_new_part(void) p = (AVRPART *)malloc(sizeof(AVRPART)); if (p == NULL) { - avrdude_message(MSG_INFO, "new_part(): out of memory\n"); - exit(1); + // avrdude_message(MSG_INFO, "new_part(): out of memory\n"); + // exit(1); + avrdude_oom("new_part(): out of memory\n"); } memset(p, 0, sizeof(*p)); diff --git a/xs/src/avrdude/buspirate.c b/xs/src/avrdude/buspirate.c index 435c4ce53..5875d4283 100644 --- a/xs/src/avrdude/buspirate.c +++ b/xs/src/avrdude/buspirate.c @@ -1135,9 +1135,10 @@ static void buspirate_setup(struct programmer_t *pgm) { /* Allocate private data */ if ((pgm->cookie = calloc(1, sizeof(struct pdata))) == 0) { - avrdude_message(MSG_INFO, "%s: buspirate_initpgm(): Out of memory allocating private data\n", - progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: buspirate_initpgm(): Out of memory allocating private data\n", + // progname); + // exit(1); + avrdude_oom("buspirate_initpgm(): Out of memory allocating private data\n"); } PDATA(pgm)->serial_recv_timeout = 100; } diff --git a/xs/src/avrdude/butterfly.c b/xs/src/avrdude/butterfly.c index de9a3175f..beb5e04de 100644 --- a/xs/src/avrdude/butterfly.c +++ b/xs/src/avrdude/butterfly.c @@ -63,9 +63,10 @@ struct pdata static void butterfly_setup(PROGRAMMER * pgm) { if ((pgm->cookie = malloc(sizeof(struct pdata))) == 0) { - avrdude_message(MSG_INFO, "%s: butterfly_setup(): Out of memory allocating private data\n", - progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: butterfly_setup(): Out of memory allocating private data\n", + // progname); + // exit(1); + avrdude_oom("butterfly_setup(): Out of memory allocating private data\n"); } memset(pgm->cookie, 0, sizeof(struct pdata)); } diff --git a/xs/src/avrdude/lexer.c b/xs/src/avrdude/lexer.c index 93249a9ab..f2d8adb4b 100644 --- a/xs/src/avrdude/lexer.c +++ b/xs/src/avrdude/lexer.c @@ -2834,7 +2834,8 @@ YY_BUFFER_STATE yy_scan_bytes (const char * yybytes, int _yybytes_len ) n = (yy_size_t) (_yybytes_len + 2); buf = (char *) yyalloc( n ); if ( ! buf ) - YY_FATAL_ERROR( "out of dynamic memory in yy_scan_bytes()" ); + // YY_FATAL_ERROR( "out of dynamic memory in yy_scan_bytes()" ); + avrdude_oom("out of dynamic memory in yy_scan_bytes()"); for ( i = 0; i < _yybytes_len; ++i ) buf[i] = yybytes[i]; @@ -2859,8 +2860,9 @@ YY_BUFFER_STATE yy_scan_bytes (const char * yybytes, int _yybytes_len ) static void yynoreturn yy_fatal_error (const char* msg ) { - fprintf( stderr, "%s\n", msg ); - exit( YY_EXIT_FAILURE ); + fprintf( stderr, "%s\n", msg ); + // exit( YY_EXIT_FAILURE ); + abort(); } /* Redefine yyless() so it works in section 3 code. */ diff --git a/xs/src/avrdude/main.c b/xs/src/avrdude/main.c index 5d73403b0..ebda0ba19 100644 --- a/xs/src/avrdude/main.c +++ b/xs/src/avrdude/main.c @@ -144,6 +144,33 @@ void avrdude_progress_external(const char *task, unsigned progress) avrdude_progress_handler(task, progress, avrdude_progress_handler_user_p); } +static void avrdude_oom_handler_null(const char *context, void *user_p) +{ + // Output a message and just exit + fputs("avrdude: Out of memory: ", stderr); + fputs(context, stderr); + exit(99); +} + +static void *avrdude_oom_handler_user_p = NULL; +static avrdude_oom_handler_t avrdude_oom_handler = avrdude_oom_handler_null; + +void avrdude_oom_handler_set(avrdude_oom_handler_t newhandler, void *user_p) +{ + if (newhandler != NULL) { + avrdude_oom_handler = newhandler; + avrdude_oom_handler_user_p = user_p; + } else { + avrdude_oom_handler = avrdude_oom_handler_null; + avrdude_oom_handler_user_p = NULL; + } +} + +void avrdude_oom(const char *context) +{ + avrdude_oom_handler(context, avrdude_oom_handler_user_p); +} + void avrdude_cancel() { cancel_flag = true; diff --git a/xs/src/avrdude/pgm.c b/xs/src/avrdude/pgm.c index 851ac5a87..b8a93f104 100644 --- a/xs/src/avrdude/pgm.c +++ b/xs/src/avrdude/pgm.c @@ -172,9 +172,10 @@ PROGRAMMER * pgm_dup(const PROGRAMMER * const src) for (ln = lfirst(src->usbpid); ln; ln = lnext(ln)) { int *ip = malloc(sizeof(int)); if (ip == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory allocating programmer structure\n", - progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory allocating programmer structure\n", + // progname); + // exit(1); + avrdude_oom("out of memory allocating programmer structure\n"); } *ip = *(int *) ldata(ln); ladd(pgm->usbpid, ip); diff --git a/xs/src/avrdude/ser_win32.c b/xs/src/avrdude/ser_win32.c index 3a05cfa90..4e1713128 100644 --- a/xs/src/avrdude/ser_win32.c +++ b/xs/src/avrdude/ser_win32.c @@ -246,10 +246,11 @@ static int ser_open(char * port, union pinfo pinfo, union filedescriptor *fdp) newname = malloc(strlen("\\\\.\\") + strlen(port) + 1); if (newname == 0) { - avrdude_message(MSG_INFO, "%s: ser_open(): out of memory\n", - progname); - exit(1); - } + // avrdude_message(MSG_INFO, "%s: ser_open(): out of memory\n", + // progname); + // exit(1); + avrdude_oom("ser_open(): out of memory\n"); + } strcpy(newname, "\\\\.\\"); strcat(newname, port); diff --git a/xs/src/avrdude/stk500v2.c b/xs/src/avrdude/stk500v2.c index 4d62640c0..691152b46 100644 --- a/xs/src/avrdude/stk500v2.c +++ b/xs/src/avrdude/stk500v2.c @@ -295,9 +295,10 @@ static int stk600_xprog_program_enable(PROGRAMMER * pgm, AVRPART * p); void stk500v2_setup(PROGRAMMER * pgm) { if ((pgm->cookie = malloc(sizeof(struct pdata))) == 0) { - avrdude_message(MSG_INFO, "%s: stk500v2_setup(): Out of memory allocating private data\n", - progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: stk500v2_setup(): Out of memory allocating private data\n", + // progname); + // exit(1); + avrdude_oom("stk500v2_setup(): Out of memory allocating private data\n"); } memset(pgm->cookie, 0, sizeof(struct pdata)); PDATA(pgm)->command_sequence = 1; diff --git a/xs/src/avrdude/update.c b/xs/src/avrdude/update.c index 417cbf71d..a255ab4f9 100644 --- a/xs/src/avrdude/update.c +++ b/xs/src/avrdude/update.c @@ -38,8 +38,9 @@ UPDATE * parse_op(char * s) upd = (UPDATE *)malloc(sizeof(UPDATE)); if (upd == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory\n", progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory\n", progname); + // exit(1); + avrdude_oom("parse_op: out of memory\n"); } i = 0; @@ -53,8 +54,9 @@ UPDATE * parse_op(char * s) upd->op = DEVICE_WRITE; upd->filename = (char *)malloc(strlen(buf) + 1); if (upd->filename == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory\n", progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory\n", progname); + // exit(1); + avrdude_oom("parse_op: out of memory\n"); } strcpy(upd->filename, buf); upd->format = FMT_AUTO; @@ -63,8 +65,9 @@ UPDATE * parse_op(char * s) upd->memtype = (char *)malloc(strlen(buf)+1); if (upd->memtype == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory\n", progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory\n", progname); + // exit(1); + avrdude_oom("parse_op: out of memory\n"); } strcpy(upd->memtype, buf); @@ -179,8 +182,9 @@ UPDATE * dup_update(UPDATE * upd) u = (UPDATE *)malloc(sizeof(UPDATE)); if (u == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory\n", progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory\n", progname); + // exit(1); + avrdude_oom("dup_update: out of memory\n"); } memcpy(u, upd, sizeof(UPDATE)); @@ -200,8 +204,9 @@ UPDATE * new_update(int op, char * memtype, int filefmt, char * filename, unsign u = (UPDATE *)malloc(sizeof(UPDATE)); if (u == NULL) { - avrdude_message(MSG_INFO, "%s: out of memory\n", progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: out of memory\n", progname); + // exit(1); + avrdude_oom("new_update: out of memory\n"); } u->memtype = strdup(memtype); diff --git a/xs/src/avrdude/wiring.c b/xs/src/avrdude/wiring.c index 395459762..562a3f17c 100644 --- a/xs/src/avrdude/wiring.c +++ b/xs/src/avrdude/wiring.c @@ -85,9 +85,10 @@ static void wiring_setup(PROGRAMMER * pgm) * Now prepare our data */ if ((mycookie = malloc(sizeof(struct wiringpdata))) == 0) { - avrdude_message(MSG_INFO, "%s: wiring_setup(): Out of memory allocating private data\n", - progname); - exit(1); + // avrdude_message(MSG_INFO, "%s: wiring_setup(): Out of memory allocating private data\n", + // progname); + // exit(1); + avrdude_oom("wiring_setup(): Out of memory allocating private data\n"); } memset(mycookie, 0, sizeof(struct wiringpdata)); WIRINGPDATA(mycookie)->snoozetime = 0; diff --git a/xs/src/slic3r/GUI/FirmwareDialog.cpp b/xs/src/slic3r/GUI/FirmwareDialog.cpp index 77e70c49b..d0cd9f8cf 100644 --- a/xs/src/slic3r/GUI/FirmwareDialog.cpp +++ b/xs/src/slic3r/GUI/FirmwareDialog.cpp @@ -551,8 +551,10 @@ void FirmwareDialog::priv::perform_upload() // because the dialog ensures it doesn't exit before the background thread is done. auto q = this->q; - this->avrdude = avrdude - .on_run([this]() { + avrdude + .on_run([this](AvrDude::Ptr avrdude) { + this->avrdude = std::move(avrdude); + try { switch (this->hex_file.device) { case HexFile::DEV_MK3: