Fix segfaults (#1368)

* Ensure JavaScript exceptions bubble up

* Fix segfault

* Fix mutex segfault

* Fix segfault caused by memory leak

If the database was being closed, and non-exclusive work was scheduled,
it overrode the lock flag such that the state became open=false locked=false
instead of open=false locked=true. This caused queued work to not be
processed, leaking memory, which causes a segfault during napi cleanup.

Make the same changes to other methods for safe measure.

src/backup.cc

@@ -31,11 +31,10 @@ void Backup::Process() {
     }
 
     while (inited && !locked && !queue.empty()) {
-        Call* call = queue.front();
+        std::unique_ptr<Call> call(queue.front());
         queue.pop();
 
         call->callback(call->baton);
-        delete call;
     }
 }
 
@@ -86,21 +85,17 @@ void Backup::CleanQueue() {
 
         // Clear out the queue so that this object can get GC'ed.
         while (!queue.empty()) {
-            Call* call = queue.front();
+            std::unique_ptr<Call> call(queue.front());
             queue.pop();
 
-            Napi::Function cb = call->baton->callback.Value();
+            std::unique_ptr<Baton> baton(call->baton);
+            Napi::Function cb = baton->callback.Value();
 
             if (inited && !cb.IsEmpty() &&
                 cb.IsFunction()) {
                 TRY_CATCH_CALL(Value(), cb, 1, argv);
                 called = true;
             }
-
-            // We don't call the actual callback, so we have to make sure that
-            // the baton gets destroyed.
-            delete call->baton;
-            delete call;
         }
 
         // When we couldn't call a callback function, emit an error on the
@@ -113,13 +108,12 @@ void Backup::CleanQueue() {
     else while (!queue.empty()) {
         // Just delete all items in the queue; we already fired an event when
         // initializing the backup failed.
-        Call* call = queue.front();
+        std::unique_ptr<Call> call(queue.front());
         queue.pop();
 
         // We don't call the actual callback, so we have to make sure that
         // the baton gets destroyed.
         delete call->baton;
-        delete call;
     }
 }
 
@@ -220,13 +214,14 @@ void Backup::Work_Initialize(napi_env e, void* data) {
 }
 
 void Backup::Work_AfterInitialize(napi_env e, napi_status status, void* data) {
-    BACKUP_INIT(InitializeBaton);
+    std::unique_ptr<InitializeBaton> baton(static_cast<InitializeBaton*>(data));
+    Backup* backup = baton->backup;
 
     Napi::Env env = backup->Env();
     Napi::HandleScope scope(env);
 
     if (backup->status != SQLITE_OK) {
-        Error(baton);
+        Error(baton.get());
         backup->FinishAll();
     }
     else {
@@ -282,7 +277,8 @@ void Backup::Work_Step(napi_env e, void* data) {
 }
 
 void Backup::Work_AfterStep(napi_env e, napi_status status, void* data) {
-    BACKUP_INIT(StepBaton);
+    std::unique_ptr<StepBaton> baton(static_cast<StepBaton*>(data));
+    Backup* backup = baton->backup;
 
     Napi::Env env = backup->Env();
     Napi::HandleScope scope(env);
@@ -294,7 +290,7 @@ void Backup::Work_AfterStep(napi_env e, napi_status status, void* data) {
     }
 
     if (backup->status != SQLITE_OK && backup->status != SQLITE_DONE) {
-        Error(baton);
+        Error(baton.get());
     }
     else {
         // Fire callbacks.
@@ -329,7 +325,8 @@ void Backup::Work_Finish(napi_env e, void* data) {
 }
 
 void Backup::Work_AfterFinish(napi_env e, napi_status status, void* data) {
-    BACKUP_INIT(Baton);
+    std::unique_ptr<Baton> baton(static_cast<Baton*>(data));
+    Backup* backup = baton->backup;
 
     Napi::Env env = backup->Env();
     Napi::HandleScope scope(env);

src/backup.h

@@ -96,7 +96,7 @@ public:
     static Napi::Object Init(Napi::Env env, Napi::Object exports);
 
     struct Baton {
-        napi_async_work request;
+        napi_async_work request = NULL;
         Backup* backup;
         Napi::FunctionReference callback;
 
@@ -105,6 +105,7 @@ public:
             callback.Reset(cb_, 1);
         }
         virtual ~Baton() {
+            if (request) napi_delete_async_work(backup->Env(), request);
             backup->Unref();
             callback.Reset();
         }

src/database.h

@@ -41,7 +41,7 @@ public:
     }
 
     struct Baton {
-        napi_async_work request;
+        napi_async_work request = NULL;
         Database* db;
         Napi::FunctionReference callback;
         int status;
@@ -55,6 +55,7 @@ public:
             }
         }
         virtual ~Baton() {
+            if (request) napi_delete_async_work(db->Env(), request);
             db->Unref();
             callback.Reset();
         }

src/macros.h

@@ -119,13 +119,14 @@ inline bool OtherIsInt(Napi::Number source) {
 
 // The Mac OS compiler complains when argv is NULL unless we
 // first assign it to a locally defined variable.
-#define TRY_CATCH_CALL(context, callback, argc, argv)                          \
+#define TRY_CATCH_CALL(context, callback, argc, argv, ...)                     \
     Napi::Value* passed_argv = argv;\
     std::vector<napi_value> args;\
     if ((argc != 0) && (passed_argv != NULL)) {\
       args.assign(passed_argv, passed_argv + argc);\
     }\
-    (callback).MakeCallback(Napi::Value(context), args);
+    Napi::Value res = (callback).MakeCallback(Napi::Value(context), args);     \
+    if (res.IsEmpty()) return __VA_ARGS__;
 
 #define WORK_DEFINITION(name)                                                  \
     Napi::Value name(const Napi::CallbackInfo& info);                          \
@@ -153,15 +154,21 @@ inline bool OtherIsInt(Napi::Number source) {
     type* baton = static_cast<type*>(data);                                    \
     Statement* stmt = baton->stmt;
 
+#define STATEMENT_MUTEX(name) \
+    if (!stmt->db->_handle) { \
+        stmt->status = SQLITE_MISUSE; \
+        stmt->message = "Database handle is closed"; \
+        return; \
+    } \
+    sqlite3_mutex* name = sqlite3_db_mutex(stmt->db->_handle);
+
 #define STATEMENT_END()                                                        \
     assert(stmt->locked);                                                      \
     assert(stmt->db->pending);                                                 \
     stmt->locked = false;                                                      \
     stmt->db->pending--;                                                       \
     stmt->Process();                                                           \
-    stmt->db->Process();                                                       \
-    napi_delete_async_work(e, baton->request);                                 \
-    delete baton;
+    stmt->db->Process();
 
 #define BACKUP_BEGIN(type)                                                     \
     assert(baton);                                                             \
@@ -189,9 +196,7 @@ inline bool OtherIsInt(Napi::Number source) {
     backup->locked = false;                                                    \
     backup->db->pending--;                                                     \
     backup->Process();                                                         \
-    backup->db->Process();                                                     \
-    napi_delete_async_work(e, baton->request);                                 \
-    delete baton;
+    backup->db->Process();
 
 #define DELETE_FIELD(field)                                                    \
     if (field != NULL) {                                                       \

src/statement.cc

@@ -42,11 +42,10 @@ void Statement::Process() {
     }
 
     while (prepared && !locked && !queue.empty()) {
-        Call* call = queue.front();
+        std::unique_ptr<Call> call(queue.front());
         queue.pop();
 
         call->callback(call->baton);
-        delete call;
     }
 }
 
@@ -133,7 +132,7 @@ void Statement::Work_Prepare(napi_env e, void* data) {
 
     // In case preparing fails, we use a mutex to make sure we get the associated
     // error message.
-    sqlite3_mutex* mtx = sqlite3_db_mutex(baton->db->_handle);
+    STATEMENT_MUTEX(mtx);
     sqlite3_mutex_enter(mtx);
 
     stmt->status = sqlite3_prepare_v2(
@@ -153,13 +152,14 @@ void Statement::Work_Prepare(napi_env e, void* data) {
 }
 
 void Statement::Work_AfterPrepare(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(PrepareBaton);
+    std::unique_ptr<PrepareBaton> baton(static_cast<PrepareBaton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_OK) {
-        Error(baton);
+        Error(baton.get());
         stmt->Finalize_();
     }
     else {
@@ -347,20 +347,21 @@ void Statement::Work_BeginBind(Baton* baton) {
 void Statement::Work_Bind(napi_env e, void* data) {
     STATEMENT_INIT(Baton);
 
-    sqlite3_mutex* mtx = sqlite3_db_mutex(stmt->db->_handle);
+    STATEMENT_MUTEX(mtx);
     sqlite3_mutex_enter(mtx);
     stmt->Bind(baton->parameters);
     sqlite3_mutex_leave(mtx);
 }
 
 void Statement::Work_AfterBind(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(Baton);
+    std::unique_ptr<Baton> baton(static_cast<Baton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_OK) {
-        Error(baton);
+        Error(baton.get());
     }
     else {
         // Fire callbacks.
@@ -399,7 +400,7 @@ void Statement::Work_Get(napi_env e, void* data) {
     STATEMENT_INIT(RowBaton);
 
     if (stmt->status != SQLITE_DONE || baton->parameters.size()) {
-        sqlite3_mutex* mtx = sqlite3_db_mutex(stmt->db->_handle);
+        STATEMENT_MUTEX(mtx);
         sqlite3_mutex_enter(mtx);
 
         if (stmt->Bind(baton->parameters)) {
@@ -420,13 +421,14 @@ void Statement::Work_Get(napi_env e, void* data) {
 }
 
 void Statement::Work_AfterGet(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(RowBaton);
+    std::unique_ptr<RowBaton> baton(static_cast<RowBaton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_ROW && stmt->status != SQLITE_DONE) {
-        Error(baton);
+        Error(baton.get());
     }
     else {
         // Fire callbacks.
@@ -469,7 +471,7 @@ void Statement::Work_BeginRun(Baton* baton) {
 void Statement::Work_Run(napi_env e, void* data) {
     STATEMENT_INIT(RunBaton);
 
-    sqlite3_mutex* mtx = sqlite3_db_mutex(stmt->db->_handle);
+    STATEMENT_MUTEX(mtx);
     sqlite3_mutex_enter(mtx);
 
     // Make sure that we also reset when there are no parameters.
@@ -493,13 +495,14 @@ void Statement::Work_Run(napi_env e, void* data) {
 }
 
 void Statement::Work_AfterRun(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(RunBaton);
+    std::unique_ptr<RunBaton> baton(static_cast<RunBaton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_ROW && stmt->status != SQLITE_DONE) {
-        Error(baton);
+        Error(baton.get());
     }
     else {
         // Fire callbacks.
@@ -538,7 +541,7 @@ void Statement::Work_BeginAll(Baton* baton) {
 void Statement::Work_All(napi_env e, void* data) {
     STATEMENT_INIT(RowsBaton);
 
-    sqlite3_mutex* mtx = sqlite3_db_mutex(stmt->db->_handle);
+    STATEMENT_MUTEX(mtx);
     sqlite3_mutex_enter(mtx);
 
     // Make sure that we also reset when there are no parameters.
@@ -562,13 +565,14 @@ void Statement::Work_All(napi_env e, void* data) {
 }
 
 void Statement::Work_AfterAll(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(RowsBaton);
+    std::unique_ptr<RowsBaton> baton(static_cast<RowsBaton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_DONE) {
-        Error(baton);
+        Error(baton.get());
     }
     else {
         // Fire callbacks.
@@ -580,8 +584,8 @@ void Statement::Work_AfterAll(napi_env e, napi_status status, void* data) {
                 Rows::const_iterator it = baton->rows.begin();
                 Rows::const_iterator end = baton->rows.end();
                 for (int i = 0; it < end; ++it, i++) {
-                    (result).Set(i, RowToJS(env,*it));
-                    delete *it;
+                    std::unique_ptr<Row> row(*it);
+                    (result).Set(i, RowToJS(env,row.get()));
                 }
 
                 Napi::Value argv[] = { env.Null(), result };
@@ -640,7 +644,7 @@ void Statement::Work_Each(napi_env e, void* data) {
 
     Async* async = baton->async;
 
-    sqlite3_mutex* mtx = sqlite3_db_mutex(stmt->db->_handle);
+    STATEMENT_MUTEX(mtx);
 
     int retrieved = 0;
 
@@ -710,10 +714,10 @@ void Statement::AsyncEach(uv_async_t* handle) {
             Rows::const_iterator it = rows.begin();
             Rows::const_iterator end = rows.end();
             for (int i = 0; it < end; ++it, i++) {
-                argv[1] = RowToJS(env,*it);
+                std::unique_ptr<Row> row(*it);
+                argv[1] = RowToJS(env,row.get());
                 async->retrieved++;
                 TRY_CATCH_CALL(async->stmt->Value(), cb, 2, argv);
-                delete *it;
             }
         }
     }
@@ -733,13 +737,14 @@ void Statement::AsyncEach(uv_async_t* handle) {
 }
 
 void Statement::Work_AfterEach(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(EachBaton);
+    std::unique_ptr<EachBaton> baton(static_cast<EachBaton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
 
     if (stmt->status != SQLITE_DONE) {
-        Error(baton);
+        Error(baton.get());
     }
 
     STATEMENT_END();
@@ -769,7 +774,8 @@ void Statement::Work_Reset(napi_env e, void* data) {
 }
 
 void Statement::Work_AfterReset(napi_env e, napi_status status, void* data) {
-    STATEMENT_INIT(Baton);
+    std::unique_ptr<Baton> baton(static_cast<Baton*>(data));
+    Statement* stmt = baton->stmt;
 
     Napi::Env env = stmt->Env();
     Napi::HandleScope scope(env);
@@ -865,7 +871,8 @@ Napi::Value Statement::Finalize_(const Napi::CallbackInfo& info) {
     return stmt->db->Value();
 }
 
-void Statement::Finalize_(Baton* baton) {
+void Statement::Finalize_(Baton* b) {
+    std::unique_ptr<Baton> baton(b);
     Napi::Env env = baton->stmt->Env();
     Napi::HandleScope scope(env);
 
@@ -876,8 +883,6 @@ void Statement::Finalize_(Baton* baton) {
     if (!cb.IsUndefined() && cb.IsFunction()) {
         TRY_CATCH_CALL(baton->stmt->Value(), cb, 0, NULL);
     }
-
-    delete baton;
 }
 
 void Statement::Finalize_() {
@@ -904,21 +909,17 @@ void Statement::CleanQueue() {
 
         // Clear out the queue so that this object can get GC'ed.
         while (!queue.empty()) {
-            Call* call = queue.front();
+            std::unique_ptr<Call> call(queue.front());
             queue.pop();
 
-            Napi::Function cb = call->baton->callback.Value();
+            std::unique_ptr<Baton> baton(call->baton);
+            Napi::Function cb = baton->callback.Value();
 
             if (prepared && !cb.IsEmpty() &&
                 cb.IsFunction()) {
                 TRY_CATCH_CALL(Value(), cb, 1, argv);
                 called = true;
             }
-
-            // We don't call the actual callback, so we have to make sure that
-            // the baton gets destroyed.
-            delete call->baton;
-            delete call;
         }
 
         // When we couldn't call a callback function, emit an error on the
@@ -931,12 +932,10 @@ void Statement::CleanQueue() {
     else while (!queue.empty()) {
         // Just delete all items in the queue; we already fired an event when
         // preparing the statement failed.
-        Call* call = queue.front();
+        std::unique_ptr<Call> call(queue.front());
         queue.pop();
-
         // We don't call the actual callback, so we have to make sure that
         // the baton gets destroyed.
         delete call->baton;
-        delete call;
     }
 }

src/statement.h

@@ -76,7 +76,7 @@ public:
     static Napi::Value New(const Napi::CallbackInfo& info);
 
     struct Baton {
-        napi_async_work request;
+        napi_async_work request = NULL;
         Statement* stmt;
         Napi::FunctionReference callback;
         Parameters parameters;
@@ -90,6 +90,7 @@ public:
                 Values::Field* field = parameters[i];
                 DELETE_FIELD(field);
             }
+            if (request) napi_delete_async_work(stmt->Env(), request);
             stmt->Unref();
             callback.Reset();
         }