Big INSERTs with unescaped parts cause SIGABRT

[oxff]

The following code triggers the bug:

var insert_values = new Array({ value: '@someid', escape: false },
  someval,
  anotherval
);
db.query().insert('table', ['`fkey_ids`', '`col1`', '`col2`'],
   new Array(insert_values, ...)).execute();

I believe the bug is due to node_db::Query::placeholders miscalulating the delta. It only occurs when INSERTing lots of rows.

terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::replace

Program received signal SIGABRT, Aborted.
0x00007ffff62cc165 in raise () from /lib/libc.so.6
(gdb) bt
#0  0x00007ffff62cc165 in raise () from /lib/libc.so.6
#1  0x00007ffff62cef70 in abort () from /lib/libc.so.6
#2  0x00007ffff6d7bdc5 in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/libstdc++.so.6
#3  0x00007ffff6d7a166 in ?? () from /usr/lib/libstdc++.so.6
#4  0x00007ffff6d7a193 in std::terminate() () from /usr/lib/libstdc++.so.6
#5  0x00007ffff6d7a1a6 in ?? () from /usr/lib/libstdc++.so.6
#6  0x00007ffff6d797e3 in __cxa_call_unexpected () from /usr/lib/libstdc++.so.6
#7  0x00007ffff588461b in node_db::Query::parseQuery (this=0xfb8ca0) at ../lib/node-db/query.cc:1378
#8  0x00007ffff587d2f8 in node_db::Query::Execute (args=...) at ../lib/node-db/query.cc:599
#9  0x00000000005a96b1 in v8::internal::Builtin_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) ()
#10 0x00002d40ce48814e in ?? ()

[oxff]

Turned out the first input to be replaced was "undefined", causing Query::value(..) to return a zero-length string. This caused an integer underflow in delta, but since it was declared as uint32_t, it was just a huge value when added to the size_t on a 64bit system. The attached patch adds additional sanity checks to prevent this from crashing node.

From 3f19e9c3298633e680b13f4de13dedfd2deae4c8 Mon Sep 17 00:00:00 2001
From: Georg Wicherski <gw@oxff.net>
Date: Wed, 30 Nov 2011 11:48:57 -0500
Subject: [PATCH] Add sanity checks in Query::value and Query::parseQuery to avoid errors on invalid user input, fixes #42 in node-db-mysql

---
 query.cc |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/query.cc b/query.cc
index b96b4c0..3a5a7b6 100644
--- a/query.cc
+++ b/query.cc
@@ -1,4 +1,5 @@
 // Copyright 2011 Mariano Iglesias <mgiglesias@gmail.com>
+// Copyright 2011 Georg Wicherski <gw@oxff.net>
 #include "./query.h"

 bool node_db::Query::gmtDeltaLoaded = false;
@@ -1382,6 +1383,11 @@ std::string node_db::Query::parseQuery() const throw(node_db::Exception&) {
     uint32_t index = 0, delta = 0;
     for (std::vector<std::string::size_type>::iterator iterator = positions.begin(), end = positions.end(); iterator != end; ++iterator, index++) {
         std::string value = this->value(*(this->values[index]));
+
+   if(!value.length()) {
+       throw node_db::Exception("Internal error, attempting to replace with zero length value");
+   }
+
         parsed.replace(*iterator + delta, 1, value);
         delta += (value.length() - 1);
     }
@@ -1479,6 +1485,10 @@ std::string node_db::Query::value(v8::Local<v8::Value> value, bool inArray, bool
         } else {
             currentStream << string;
         }
+    } else {   
+        v8::String::Utf8Value currentString(value->ToString());
+        std::string string = *currentString;
+        throw node_db::Exception("Unknown type for to convert to SQL, converting `" + string + "'");
     }

     return currentStream.str();
-- 
1.7.2.5

[mariano]

@oxff: awesome job with the patch, thanks!

Can you make it a pull request?

[oxff]

On 11/30/2011 07:18 PM, Mariano Iglesias wrote:

Can you make it a pull request?

I don't have a correspondending branch / don't want to create a github
repository. You can simply save the attached forwarded message to a file
and use git-am to import it into your local repository, git am <path-to-file> should work.

Let me know if you run into any troubles for that. :)

Thanks for accepting the patch,
Georg