diff --git a/CGIFormData.h b/CGIFormData.h index c3e022d..1ae46b0 100644 --- a/CGIFormData.h +++ b/CGIFormData.h @@ -34,6 +34,14 @@ namespace coreutils { MString operator[](ZString name); + // Every parsed field, keyed as "name:N" (see storeValue's own doc + // comment below) - lets a caller (ApiSession::handleComplete, to + // merge POST body fields into jet::Global::variables the same way + // path params/query string already are - see its own doc comment) + // iterate every field without knowing its name in advance, which + // operator[] alone doesn't support. + const std::map &fields() const { return data; } + protected: Status parse() override; diff --git a/File.cpp b/File.cpp index 51a43f6..b90d481 100644 --- a/File.cpp +++ b/File.cpp @@ -17,19 +17,34 @@ namespace coreutils { this->fileName = fileName; - struct stat status; - stat(fileName.c_str(), &status); - buffer = NULL; - setBufferSize(status.st_size); - fd = ::open(fileName.c_str(), mode, authority); if(fd < 0) { std::stringstream temp; temp << "Error opening file " << fileName << "."; throw Exception(temp.str(), __FILE__, __LINE__); } + + // stat() only after a successful open(), and only used to size the + // read buffer - previously this ran BEFORE the open()/fd check with + // its return value ignored, so a missing file left the stat struct fully + // uninitialized and fed that garbage st_size straight into + // setBufferSize()'s realloc(), before the open-failure path ever + // got a chance to throw its clean Exception. Found while adding a + // static-asset route (GET /assets/{filename}) that legitimately + // hits this path on every request for a nonexistent file - a + // garbage-sized realloc there could abort/crash the whole + // long-running APIServer process (shared with every other live + // route) instead of cleanly 500ing just that one request. + struct stat status; + if (fstat(fd, &status) != 0) { + std::stringstream temp; + temp << "Error statting file " << fileName << "."; + throw Exception(temp.str(), __FILE__, __LINE__); + } + + setBufferSize(status.st_size); } File::~File() { diff --git a/IMFMessage.cpp b/IMFMessage.cpp index dd4cea1..69dd061 100644 --- a/IMFMessage.cpp +++ b/IMFMessage.cpp @@ -7,5 +7,6 @@ namespace coreutils { IMFMessage::IMFMessage(int fd) {} + IMFMessage::~IMFMessage() {} } diff --git a/JString.cpp b/JString.cpp index d75f17f..e91f0aa 100644 --- a/JString.cpp +++ b/JString.cpp @@ -474,7 +474,11 @@ namespace coreutils { formatted << value; else formatted << "\"" << jsonEscape(value).c_str() << "\""; + setFormatted(path, formatted); + } + void JString::setFormatted(ZString path, MString formatted) { + reset(); if (locate(path)) { int startOffset = offset(); skipValue(); @@ -491,8 +495,28 @@ namespace coreutils { } void JString::setString(ZString path, std::string value) { + // Deliberately NOT routed through set() (which calls isLiteralValue() + // to guess whether the raw text should be quoted) - setString() is + // explicitly, unambiguously a string setter, so the value must + // ALWAYS be quoted+escaped regardless of what it looks like. + // isLiteralValue() classifies anything ZString::startsWithNumber() + // accepts as a bare numeric literal - true for "42", but ALSO true + // for a string that merely STARTS with digits, like a UUID + // ("00146df8-e42c-11e7-9c78-525400b869dc"). Going through set() + // wrote that UUID completely unquoted; skipValue() then only + // recognizes the JSON-number PREFIX ("00146") as the value's real + // span when locating it for a later update, so the next setString() + // call spliced its own new key/value pair in right after "00146", + // leaving "df8-e42c-...-869dc" as orphaned text outside the object + // entirely - malformed JSON, confirmed via a real corrupted session + // file (jet-2.0's own native $[@name] session variables, backed by + // this class - see Global.h). Every other setString() caller + // sitewide was silently exposed to the same risk for any string + // value that happens to start with a digit. MString v(value); - set(path, v); + MString formatted; + formatted << "\"" << jsonEscape(v).c_str() << "\""; + setFormatted(path, formatted); } void JString::setNumber(ZString path, double value) { diff --git a/JString.h b/JString.h index f1ba16c..f63fd4f 100644 --- a/JString.h +++ b/JString.h @@ -56,6 +56,18 @@ namespace coreutils { void set(ZString &path, ZString &value); + /// + /// Inserts/replaces path with already-formatted, valid JSON text + /// (a quoted+escaped string, or a bare number/bool/null literal) - + /// the locate()/skipValue()/insert() mechanics set() and setString() + /// both share, factored out so setString() can force string quoting + /// unconditionally instead of going through set()'s own + /// isLiteralValue() guess (see setString()'s own comment in the + /// .cpp for why that guess is wrong for a string setter). + /// + + void setFormatted(ZString path, MString formatted); + /// /// Use the find method to look through an object for member specified by /// the key in parameter 1. The cursor must be pointing to the { character diff --git a/MString.cpp b/MString.cpp index 3f83b2f..74ced36 100644 --- a/MString.cpp +++ b/MString.cpp @@ -266,15 +266,26 @@ namespace coreutils { } void MString::remove(int length) { + // Shifts the tail (everything after the removed range) down to close + // the gap. The loop must stop once ix + length reaches the end of the + // valid data - reading cursor[ix + length] past that point is a + // heap-buffer-overflow (confirmed via AddressSanitizer under + // concurrent APIServer load: a completely full MString hit exactly + // this on the final iteration, reading 1 byte past the allocation). + // The old condition `ix < len` let the loop run `length` iterations + // too far. int len = (data + this->length - cursor); - for (int ix = 0; ix < len; ix++) + for (int ix = 0; ix < len - length; ix++) cursor[ix] = cursor[ix + length]; setSize(this->length - length); onChange(); } void MString::remove(int offset, int length) { - for (int ix = offset; ix < this->length; ix++) + // Same off-by-one as the single-argument overload above - the loop + // must stop once ix + length reaches this->length, not run all the + // way to this->length itself. + for (int ix = offset; ix < this->length - length; ix++) getData()[ix] = getData()[ix + length]; setSize(this->length - length); onChange();