From 0510edeebf76b8a9753acc06afde8b09a8f00a54 Mon Sep 17 00:00:00 2001 From: Florin Malita Date: Thu, 14 Jun 2018 18:41:39 +0000 Subject: Revert "[skjson] Size-constrained input API" This reverts commit fc792b8718cc30e9da62c9559b23c1baac3166bb. Reason for revert: New ASAN, Android failures. Original change's description: > [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 > Commit-Queue: Florin Malita TBR=mtklein@google.com,fmalita@chromium.org Change-Id: Ic0b52398b1ce6f64c781debb858829cb64bbae58 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/135022 Reviewed-by: Florin Malita Commit-Queue: Florin Malita --- modules/skjson/include/SkJSON.h | 2 +- modules/skjson/src/FuzzSkJSON.cpp | 10 +++++++- modules/skjson/src/SkJSON.cpp | 48 ++++++++++++++------------------------ modules/skjson/src/SkJSONBench.cpp | 18 +++++++++----- modules/skjson/src/SkJSONTest.cpp | 13 ++--------- 5 files changed, 41 insertions(+), 50 deletions(-) diff --git a/modules/skjson/include/SkJSON.h b/modules/skjson/include/SkJSON.h index 1b46c4a100..99da29de72 100644 --- a/modules/skjson/include/SkJSON.h +++ b/modules/skjson/include/SkJSON.h @@ -326,7 +326,7 @@ private: class DOM final : public SkNoncopyable { public: - DOM(const char*, size_t); + explicit DOM(const char*); const Value& root() const { return fRoot; } diff --git a/modules/skjson/src/FuzzSkJSON.cpp b/modules/skjson/src/FuzzSkJSON.cpp index 2e971ce249..ce33cc1299 100644 --- a/modules/skjson/src/FuzzSkJSON.cpp +++ b/modules/skjson/src/FuzzSkJSON.cpp @@ -5,12 +5,20 @@ * found in the LICENSE file. */ +#include "SkAutoMalloc.h" #include "SkData.h" #include "SkJSON.h" #include "SkStream.h" void FuzzSkJSON(sk_sp bytes) { - skjson::DOM dom(static_cast(bytes->data()), bytes->size()); + // TODO: add a size + len skjson::DOM factory? + SkAutoMalloc data(bytes->size() + 1); + auto* c_str = static_cast(data.get()); + + memcpy(c_str, bytes->data(), bytes->size()); + c_str[bytes->size()] = '\0'; + + skjson::DOM dom(c_str); SkDynamicMemoryWStream wstream; dom.write(&wstream); } diff --git a/modules/skjson/src/SkJSON.cpp b/modules/skjson/src/SkJSON.cpp index 64f1302a7e..bf176c5c5c 100644 --- a/modules/skjson/src/SkJSON.cpp +++ b/modules/skjson/src/SkJSON.cpp @@ -11,12 +11,12 @@ #include "SkString.h" #include -#include #include namespace skjson { -// #define SK_JSON_REPORT_ERRORS +//#define SK_JSON_REPORT_ERRORS + static_assert( sizeof(Value) == 8, ""); static_assert(alignof(Value) == 8, ""); @@ -236,21 +236,7 @@ public: fScopeStack.reserve(kScopeStackReserve); } - 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"); - } - + const Value parse(const char* p) { p = skip_ws(p); switch (*p) { @@ -354,17 +340,17 @@ public: pop_common: SkASSERT(*p == '}' || *p == ']'); + ++p; + if (fScopeStack.empty()) { SkASSERT(fValueStack.size() == 1); - // Success condition: parsed the top level element and reached the stop token. - return p == p_stop + // Stop condition: parsed the top level element and there is no trailing garbage. + return *skip_ws(p) == '\0' ? fValueStack.front() - : this->error(NullValue(), p + 1, "trailing root garbage"); + : this->error(NullValue(), p, "trailing root garbage"); } - ++p; - goto match_post_value; match_array: @@ -391,20 +377,19 @@ public: return NullValue(); } - std::tuple getError() const { - return std::make_tuple(fErrorToken, fErrorMessage); + const SkString& getError() const { + return fError; } private: - SkArenaAlloc& fAlloc; + SkArenaAlloc& fAlloc; static constexpr size_t kValueStackReserve = 256; static constexpr size_t kScopeStackReserve = 128; std::vector fValueStack; std::vector fScopeStack; - const char* fErrorToken = nullptr; - SkString fErrorMessage; + SkString fError; template void popScopeAsVec(size_t scope_start) { @@ -505,8 +490,9 @@ private: template T error(T&& ret_val, const char* p, const char* msg) { #if defined(SK_JSON_REPORT_ERRORS) - fErrorToken = p; - fErrorMessage.set(msg); + static constexpr size_t kMaxContext = 128; + fError = SkStringPrintf("%s: >", msg); + fError.append(p, std::min(strlen(p), kMaxContext)); #endif return ret_val; } @@ -732,11 +718,11 @@ SkString Value::toString() const { static constexpr size_t kMinChunkSize = 4096; -DOM::DOM(const char* data, size_t size) +DOM::DOM(const char* c_str) : fAlloc(kMinChunkSize) { DOMParser parser(fAlloc); - fRoot = parser.parse(data, size); + fRoot = parser.parse(c_str); } void DOM::write(SkWStream* stream) const { diff --git a/modules/skjson/src/SkJSONBench.cpp b/modules/skjson/src/SkJSONBench.cpp index b9c3664306..4be3b972ab 100644 --- a/modules/skjson/src/SkJSONBench.cpp +++ b/modules/skjson/src/SkJSONBench.cpp @@ -25,21 +25,27 @@ protected: bool isSuitableFor(Backend backend) override { return backend == kNonRendering_Backend; } void onPerCanvasPreDraw(SkCanvas*) override { - fData = SkData::MakeFromFileName(kBenchFile); - if (!fData) { + if (auto stream = SkStream::MakeFromFile(kBenchFile)) { + SkASSERT(stream->hasLength()); + fCStringData = SkData::MakeUninitialized(stream->getLength() + 1); + auto* data8 = reinterpret_cast(fCStringData->writable_data()); + SkAssertResult(stream->read(data8, stream->getLength()) == stream->getLength()); + data8[stream->getLength()] = '\0'; + + } else { SkDebugf("!! Could not open bench file: %s\n", kBenchFile); } } void onPerCanvasPostDraw(SkCanvas*) override { - fData = nullptr; + fCStringData = nullptr; } void onDraw(int loops, SkCanvas*) override { - if (!fData) return; + if (!fCStringData) return; for (int i = 0; i < loops; i++) { - skjson::DOM dom(static_cast(fData->data()), fData->size()); + skjson::DOM dom(static_cast(fCStringData->data())); if (dom.root().is()) { SkDebugf("!! Parsing failed.\n"); return; @@ -48,7 +54,7 @@ protected: } private: - sk_sp fData; + sk_sp fCStringData; using INHERITED = Benchmark; }; diff --git a/modules/skjson/src/SkJSONTest.cpp b/modules/skjson/src/SkJSONTest.cpp index 91ae497cf0..6d4338cbbb 100644 --- a/modules/skjson/src/SkJSONTest.cpp +++ b/modules/skjson/src/SkJSONTest.cpp @@ -22,14 +22,8 @@ DEF_TEST(SkJSON_Parse, reporter) { { "" , nullptr }, { "[" , nullptr }, { "]" , nullptr }, - { "[[]" , nullptr }, - { "[]]" , nullptr }, - { "[]f" , nullptr }, { "{" , nullptr }, { "}" , nullptr }, - { "{{}" , nullptr }, - { "{}}" , nullptr }, - { "{}f" , nullptr }, { "{]" , nullptr }, { "[}" , nullptr }, { "1" , nullptr }, @@ -57,9 +51,7 @@ DEF_TEST(SkJSON_Parse, reporter) { { "{ \"k\" : null \"k\" : 1 }", nullptr }, - { "[]" , "[]" }, { " \n\r\t [ \n\r\t ] \n\r\t " , "[]" }, - { "[[]]" , "[[]]" }, { "[ null ]" , "[null]" }, { "[ true ]" , "[true]" }, { "[ false ]" , "[false]" }, @@ -74,7 +66,6 @@ DEF_TEST(SkJSON_Parse, reporter) { { "[ \"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}" }, @@ -98,7 +89,7 @@ DEF_TEST(SkJSON_Parse, reporter) { }; for (const auto& tst : g_tests) { - DOM dom(tst.in, strlen(tst.in)); + DOM dom(tst.in); const auto success = !dom.root().is(); REPORTER_ASSERT(reporter, success == (tst.out != nullptr)); if (!success) continue; @@ -162,7 +153,7 @@ DEF_TEST(SkJSON_DOM_visit, reporter) { \"k8\": { \"kk1\": 2, \"kk2\": false, \"kk1\": \"baz\" } \n\ }"; - DOM dom(json, strlen(json)); + DOM dom(json); const auto& jroot = dom.root().as(); REPORTER_ASSERT(reporter, jroot.is()); -- cgit v1.2.3