[skjson] Size-constrained input API
Pass an explicit input size instead of requiring a C string.
Thanks to mtklein's clever trick, this has no measurable perf impact.
Change-Id: I64f210a9f653a78b05ab6b58fa34479504aa35ff
Reviewed-on: https://skia-review.googlesource.com/134940
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/modules/skjson/include/SkJSON.h b/modules/skjson/include/SkJSON.h
index 99da29d..1b46c4a 100644
--- a/modules/skjson/include/SkJSON.h
+++ b/modules/skjson/include/SkJSON.h
@@ -326,7 +326,7 @@
class DOM final : public SkNoncopyable {
public:
- explicit DOM(const char*);
+ DOM(const char*, size_t);
const Value& root() const { return fRoot; }
diff --git a/modules/skjson/src/FuzzSkJSON.cpp b/modules/skjson/src/FuzzSkJSON.cpp
index ce33cc1..2e971ce 100644
--- a/modules/skjson/src/FuzzSkJSON.cpp
+++ b/modules/skjson/src/FuzzSkJSON.cpp
@@ -5,20 +5,12 @@
* found in the LICENSE file.
*/
-#include "SkAutoMalloc.h"
#include "SkData.h"
#include "SkJSON.h"
#include "SkStream.h"
void FuzzSkJSON(sk_sp<SkData> bytes) {
- // TODO: add a size + len skjson::DOM factory?
- SkAutoMalloc data(bytes->size() + 1);
- auto* c_str = static_cast<char*>(data.get());
-
- memcpy(c_str, bytes->data(), bytes->size());
- c_str[bytes->size()] = '\0';
-
- skjson::DOM dom(c_str);
+ skjson::DOM dom(static_cast<const char*>(bytes->data()), bytes->size());
SkDynamicMemoryWStream wstream;
dom.write(&wstream);
}
diff --git a/modules/skjson/src/SkJSON.cpp b/modules/skjson/src/SkJSON.cpp
index b513c78..e0a8f43 100644
--- a/modules/skjson/src/SkJSON.cpp
+++ b/modules/skjson/src/SkJSON.cpp
@@ -11,12 +11,12 @@
#include "SkString.h"
#include <cmath>
+#include <tuple>
#include <vector>
namespace skjson {
-//#define SK_JSON_REPORT_ERRORS
-
+// #define SK_JSON_REPORT_ERRORS
static_assert( sizeof(Value) == 8, "");
static_assert(alignof(Value) == 8, "");
@@ -231,7 +231,21 @@
fScopeStack.reserve(kScopeStackReserve);
}
- const Value parse(const char* p) {
+ const Value parse(const char* p, size_t size) {
+ if (!size) {
+ this->error(NullValue(), p, "invalid empty input");
+ }
+
+ const char* p_stop = p + size - 1;
+
+ // We're only checking for end-of-stream on object/array close('}',']'),
+ // so we must trim any whitespace from the buffer tail.
+ while (p_stop > p && is_ws(*p_stop)) --p_stop;
+
+ if (*p_stop != '}' && *p_stop != ']') {
+ return this->error(NullValue(), p_stop, "invalid top-level value");
+ }
+
p = skip_ws(p);
switch (*p) {
@@ -335,17 +349,17 @@
pop_common:
SkASSERT(*p == '}' || *p == ']');
- ++p;
-
if (fScopeStack.empty()) {
SkASSERT(fValueStack.size() == 1);
- // Stop condition: parsed the top level element and there is no trailing garbage.
- return *skip_ws(p) == '\0'
+ // Success condition: parsed the top level element and reached the stop token.
+ return p == p_stop
? fValueStack.front()
- : this->error(NullValue(), p, "trailing root garbage");
+ : this->error(NullValue(), p + 1, "trailing root garbage");
}
+ ++p;
+
goto match_post_value;
match_array:
@@ -372,19 +386,20 @@
return NullValue();
}
- const SkString& getError() const {
- return fError;
+ std::tuple<const char*, const SkString> getError() const {
+ return std::make_tuple(fErrorToken, fErrorMessage);
}
private:
- SkArenaAlloc& fAlloc;
+ SkArenaAlloc& fAlloc;
static constexpr size_t kValueStackReserve = 256;
static constexpr size_t kScopeStackReserve = 128;
std::vector<Value > fValueStack;
std::vector<intptr_t> fScopeStack;
- SkString fError;
+ const char* fErrorToken = nullptr;
+ SkString fErrorMessage;
template <typename VectorT>
void popScopeAsVec(size_t scope_start) {
@@ -485,9 +500,8 @@
template <typename T>
T error(T&& ret_val, const char* p, const char* msg) {
#if defined(SK_JSON_REPORT_ERRORS)
- static constexpr size_t kMaxContext = 128;
- fError = SkStringPrintf("%s: >", msg);
- fError.append(p, std::min(strlen(p), kMaxContext));
+ fErrorToken = p;
+ fErrorMessage.set(msg);
#endif
return ret_val;
}
@@ -713,11 +727,11 @@
static constexpr size_t kMinChunkSize = 4096;
-DOM::DOM(const char* c_str)
+DOM::DOM(const char* data, size_t size)
: fAlloc(kMinChunkSize) {
DOMParser parser(fAlloc);
- fRoot = parser.parse(c_str);
+ fRoot = parser.parse(data, size);
}
void DOM::write(SkWStream* stream) const {
diff --git a/modules/skjson/src/SkJSONBench.cpp b/modules/skjson/src/SkJSONBench.cpp
index 4be3b97..b9c3664 100644
--- a/modules/skjson/src/SkJSONBench.cpp
+++ b/modules/skjson/src/SkJSONBench.cpp
@@ -25,27 +25,21 @@
bool isSuitableFor(Backend backend) override { return backend == kNonRendering_Backend; }
void onPerCanvasPreDraw(SkCanvas*) override {
- if (auto stream = SkStream::MakeFromFile(kBenchFile)) {
- SkASSERT(stream->hasLength());
- fCStringData = SkData::MakeUninitialized(stream->getLength() + 1);
- auto* data8 = reinterpret_cast<uint8_t*>(fCStringData->writable_data());
- SkAssertResult(stream->read(data8, stream->getLength()) == stream->getLength());
- data8[stream->getLength()] = '\0';
-
- } else {
+ fData = SkData::MakeFromFileName(kBenchFile);
+ if (!fData) {
SkDebugf("!! Could not open bench file: %s\n", kBenchFile);
}
}
void onPerCanvasPostDraw(SkCanvas*) override {
- fCStringData = nullptr;
+ fData = nullptr;
}
void onDraw(int loops, SkCanvas*) override {
- if (!fCStringData) return;
+ if (!fData) return;
for (int i = 0; i < loops; i++) {
- skjson::DOM dom(static_cast<const char*>(fCStringData->data()));
+ skjson::DOM dom(static_cast<const char*>(fData->data()), fData->size());
if (dom.root().is<skjson::NullValue>()) {
SkDebugf("!! Parsing failed.\n");
return;
@@ -54,7 +48,7 @@
}
private:
- sk_sp<SkData> fCStringData;
+ sk_sp<SkData> fData;
using INHERITED = Benchmark;
};
diff --git a/modules/skjson/src/SkJSONTest.cpp b/modules/skjson/src/SkJSONTest.cpp
index 6d4338c..91ae497 100644
--- a/modules/skjson/src/SkJSONTest.cpp
+++ b/modules/skjson/src/SkJSONTest.cpp
@@ -22,8 +22,14 @@
{ "" , nullptr },
{ "[" , nullptr },
{ "]" , nullptr },
+ { "[[]" , nullptr },
+ { "[]]" , nullptr },
+ { "[]f" , nullptr },
{ "{" , nullptr },
{ "}" , nullptr },
+ { "{{}" , nullptr },
+ { "{}}" , nullptr },
+ { "{}f" , nullptr },
{ "{]" , nullptr },
{ "[}" , nullptr },
{ "1" , nullptr },
@@ -51,7 +57,9 @@
{ "{ \"k\" : null \"k\" : 1 }", nullptr },
+ { "[]" , "[]" },
{ " \n\r\t [ \n\r\t ] \n\r\t " , "[]" },
+ { "[[]]" , "[[]]" },
{ "[ null ]" , "[null]" },
{ "[ true ]" , "[true]" },
{ "[ false ]" , "[false]" },
@@ -66,6 +74,7 @@
{ "[ \"123456789\" ]" , "[\"123456789\"]" },
{ "[ null , true, false,0,12.8 ]", "[null,true,false,0,12.8]" },
+ { "{}" , "{}" },
{ " \n\r\t { \n\r\t } \n\r\t " , "{}" },
{ "{ \"k\" : null }" , "{\"k\":null}" },
{ "{ \"k1\" : null, \"k2 \":0 }", "{\"k1\":null,\"k2 \":0}" },
@@ -89,7 +98,7 @@
};
for (const auto& tst : g_tests) {
- DOM dom(tst.in);
+ DOM dom(tst.in, strlen(tst.in));
const auto success = !dom.root().is<NullValue>();
REPORTER_ASSERT(reporter, success == (tst.out != nullptr));
if (!success) continue;
@@ -153,7 +162,7 @@
\"k8\": { \"kk1\": 2, \"kk2\": false, \"kk1\": \"baz\" } \n\
}";
- DOM dom(json);
+ DOM dom(json, strlen(json));
const auto& jroot = dom.root().as<ObjectValue>();
REPORTER_ASSERT(reporter, jroot.is<ObjectValue>());