From 1ec850383bb82f6d8bebc7416e5f50b649d1eeaa Mon Sep 17 00:00:00 2001 From: erikcorry Date: Mon, 30 Mar 2015 02:55:17 -0700 Subject: [PATCH] Fix JSON parser Handle leak R=verwaest@chromium.org BUG=v8:3976 LOG=y Review URL: https://codereview.chromium.org/1041483004 Cr-Commit-Position: refs/heads/master@{#27512} --- src/flag-definitions.h | 2 + src/heap/heap.cc | 19 +++ src/heap/heap.h | 1 + src/json-parser.h | 203 ++++++++++++++++----------- test/mjsunit/regress/regress-3976.js | 2 +- 5 files changed, 143 insertions(+), 84 deletions(-) diff --git a/src/flag-definitions.h b/src/flag-definitions.h index f1bdc0c29..42d8ce3cb 100644 --- a/src/flag-definitions.h +++ b/src/flag-definitions.h @@ -844,6 +844,8 @@ DEFINE_BOOL(gc_verbose, false, "print stuff during garbage collection") DEFINE_BOOL(heap_stats, false, "report heap statistics before and after GC") DEFINE_BOOL(code_stats, false, "report code statistics after GC") DEFINE_BOOL(print_handles, false, "report handles after GC") +DEFINE_BOOL(check_handle_count, false, + "Check that there are not too many handles at GC") DEFINE_BOOL(print_global_handles, false, "report global handles after GC") // TurboFan debug-only flags. diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 8e7a0bb85..edea4736b 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -618,6 +618,7 @@ void Heap::GarbageCollectionEpilogue() { if (FLAG_gc_verbose) Print(); if (FLAG_code_stats) ReportCodeStatistics("After GC"); #endif + if (FLAG_check_handle_count) CheckHandleCount(); if (FLAG_deopt_every_n_garbage_collections > 0) { // TODO(jkummerow/ulan/jarin): This is not safe! We can't assume that // the topmost optimized frame can be deoptimized safely, because it @@ -5650,6 +5651,24 @@ void Heap::PrintHandles() { #endif +class CheckHandleCountVisitor : public ObjectVisitor { + public: + CheckHandleCountVisitor() : handle_count_(0) {} + ~CheckHandleCountVisitor() { CHECK(handle_count_ < 1000); } + void VisitPointers(Object** start, Object** end) { + handle_count_ += end - start; + } + + private: + ptrdiff_t handle_count_; +}; + + +void Heap::CheckHandleCount() { + CheckHandleCountVisitor v; + isolate_->handle_scope_implementer()->Iterate(&v); +} + Space* AllSpaces::next() { switch (counter_++) { diff --git a/src/heap/heap.h b/src/heap/heap.h index 288bc09a1..8f5551aa5 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -983,6 +983,7 @@ class Heap { } static bool RootIsImmortalImmovable(int root_index); + void CheckHandleCount(); #ifdef VERIFY_HEAP // Verify the heap is in its normal state before or after a GC. diff --git a/src/json-parser.h b/src/json-parser.h index 1a9a36f28..f0d9e4b8b 100644 --- a/src/json-parser.h +++ b/src/json-parser.h @@ -155,6 +155,10 @@ class JsonParser BASE_EMBEDDED { // JavaScript array. Handle ParseJsonObject(); + // Helper for ParseJsonObject. Parses the form "123": obj, which is recorded + // as an element, not a property. + bool ParseElement(Handle json_object); + // Parses a JSON array literal (grammar production JSONArray). An array // literal is a square-bracketed and comma separated sequence (possibly empty) // of JSON values. @@ -299,6 +303,38 @@ Handle JsonParser::ParseJsonValue() { } +template +bool JsonParser::ParseElement(Handle json_object) { + uint32_t index = 0; + // Maybe an array index, try to parse it. + if (c0_ == '0') { + // With a leading zero, the string has to be "0" only to be an index. + Advance(); + } else { + do { + int d = c0_ - '0'; + if (index > 429496729U - ((d > 5) ? 1 : 0)) break; + index = (index * 10) + d; + Advance(); + } while (IsDecimalDigit(c0_)); + } + + if (c0_ == '"') { + // Successfully parsed index, parse and store element. + AdvanceSkipWhitespace(); + + if (c0_ == ':') { + AdvanceSkipWhitespace(); + Handle value = ParseJsonValue(); + if (!value.is_null()) { + JSObject::SetOwnElement(json_object, index, value, SLOPPY).Assert(); + return true; + } + } + } + return false; +} + // Parse a JSON object. Position must be right at '{'. template Handle JsonParser::ParseJsonObject() { @@ -320,35 +356,10 @@ Handle JsonParser::ParseJsonObject() { int start_position = position_; Advance(); - uint32_t index = 0; if (IsDecimalDigit(c0_)) { - // Maybe an array index, try to parse it. - if (c0_ == '0') { - // With a leading zero, the string has to be "0" only to be an index. - Advance(); - } else { - do { - int d = c0_ - '0'; - if (index > 429496729U - ((d > 5) ? 1 : 0)) break; - index = (index * 10) + d; - Advance(); - } while (IsDecimalDigit(c0_)); - } - - if (c0_ == '"') { - // Successfully parsed index, parse and store element. - AdvanceSkipWhitespace(); - - if (c0_ != ':') return ReportUnexpectedCharacter(); - AdvanceSkipWhitespace(); - Handle value = ParseJsonValue(); - if (value.is_null()) return ReportUnexpectedCharacter(); - - JSObject::SetOwnElement(json_object, index, value, SLOPPY).Assert(); - continue; - } - // Not an index, fallback to the slow path. + if (ParseElement(json_object)) continue; } + // Not an index, fallback to the slow path. position_ = start_position; #ifdef DEBUG @@ -360,81 +371,107 @@ Handle JsonParser::ParseJsonObject() { // Try to follow existing transitions as long as possible. Once we stop // transitioning, no transition can be found anymore. + DCHECK(transitioning); + // First check whether there is a single expected transition. If so, try + // to parse it first. + bool follow_expected = false; + Handle target; + if (seq_one_byte) { + key = TransitionArray::ExpectedTransitionKey(map); + follow_expected = !key.is_null() && ParseJsonString(key); + } + // If the expected transition hits, follow it. + if (follow_expected) { + target = TransitionArray::ExpectedTransitionTarget(map); + } else { + // If the expected transition failed, parse an internalized string and + // try to find a matching transition. + key = ParseJsonInternalizedString(); + if (key.is_null()) return ReportUnexpectedCharacter(); + + target = TransitionArray::FindTransitionToField(map, key); + // If a transition was found, follow it and continue. + transitioning = !target.is_null(); + } + if (c0_ != ':') return ReportUnexpectedCharacter(); + + AdvanceSkipWhitespace(); + value = ParseJsonValue(); + if (value.is_null()) return ReportUnexpectedCharacter(); + if (transitioning) { - // First check whether there is a single expected transition. If so, try - // to parse it first. - bool follow_expected = false; - Handle target; - if (seq_one_byte) { - key = TransitionArray::ExpectedTransitionKey(map); - follow_expected = !key.is_null() && ParseJsonString(key); - } - // If the expected transition hits, follow it. - if (follow_expected) { - target = TransitionArray::ExpectedTransitionTarget(map); + PropertyDetails details = + target->instance_descriptors()->GetDetails(descriptor); + Representation expected_representation = details.representation(); + + if (value->FitsRepresentation(expected_representation)) { + if (expected_representation.IsHeapObject() && + !target->instance_descriptors() + ->GetFieldType(descriptor) + ->NowContains(value)) { + Handle value_type( + value->OptimalType(isolate(), expected_representation)); + Map::GeneralizeFieldType(target, descriptor, + expected_representation, value_type); + } + DCHECK(target->instance_descriptors() + ->GetFieldType(descriptor) + ->NowContains(value)); + properties.Add(value, zone()); + map = target; + descriptor++; + continue; } else { - // If the expected transition failed, parse an internalized string and - // try to find a matching transition. - key = ParseJsonInternalizedString(); - if (key.is_null()) return ReportUnexpectedCharacter(); - - target = TransitionArray::FindTransitionToField(map, key); - // If a transition was found, follow it and continue. - transitioning = !target.is_null(); + transitioning = false; } - if (c0_ != ':') return ReportUnexpectedCharacter(); + } - AdvanceSkipWhitespace(); - value = ParseJsonValue(); - if (value.is_null()) return ReportUnexpectedCharacter(); + DCHECK(!transitioning); - if (transitioning) { - PropertyDetails details = - target->instance_descriptors()->GetDetails(descriptor); - Representation expected_representation = details.representation(); + // Commit the intermediate state to the object and stop transitioning. + CommitStateToJsonObject(json_object, map, &properties); - if (value->FitsRepresentation(expected_representation)) { - if (expected_representation.IsHeapObject() && - !target->instance_descriptors() - ->GetFieldType(descriptor) - ->NowContains(value)) { - Handle value_type(value->OptimalType( - isolate(), expected_representation)); - Map::GeneralizeFieldType(target, descriptor, - expected_representation, value_type); - } - DCHECK(target->instance_descriptors()->GetFieldType( - descriptor)->NowContains(value)); - properties.Add(value, zone()); - map = target; - descriptor++; - continue; - } else { - transitioning = false; - } + Runtime::DefineObjectProperty(json_object, key, value, NONE).Check(); + } while (transitioning && MatchSkipWhiteSpace(',')); + + // If we transitioned until the very end, transition the map now. + if (transitioning) { + CommitStateToJsonObject(json_object, map, &properties); + } else { + while (MatchSkipWhiteSpace(',')) { + HandleScope local_scope(isolate()); + if (c0_ != '"') return ReportUnexpectedCharacter(); + + int start_position = position_; + Advance(); + + if (IsDecimalDigit(c0_)) { + if (ParseElement(json_object)) continue; } + // Not an index, fallback to the slow path. + + position_ = start_position; +#ifdef DEBUG + c0_ = '"'; +#endif + + Handle key; + Handle value; - // Commit the intermediate state to the object and stop transitioning. - CommitStateToJsonObject(json_object, map, &properties); - } else { key = ParseJsonInternalizedString(); if (key.is_null() || c0_ != ':') return ReportUnexpectedCharacter(); AdvanceSkipWhitespace(); value = ParseJsonValue(); if (value.is_null()) return ReportUnexpectedCharacter(); + + Runtime::DefineObjectProperty(json_object, key, value, NONE).Check(); } + } - Runtime::DefineObjectProperty(json_object, key, value, NONE).Check(); - } while (MatchSkipWhiteSpace(',')); if (c0_ != '}') { return ReportUnexpectedCharacter(); } - - // If we transitioned until the very end, transition the map now. - if (transitioning) { - CommitStateToJsonObject(json_object, map, &properties); - } } AdvanceSkipWhitespace(); return scope.CloseAndEscape(json_object); diff --git a/test/mjsunit/regress/regress-3976.js b/test/mjsunit/regress/regress-3976.js index c151f689f..efa3ac03b 100644 --- a/test/mjsunit/regress/regress-3976.js +++ b/test/mjsunit/regress/regress-3976.js @@ -25,7 +25,7 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// Flags: --max-old-space-size=60 +// Flags: --max-old-space-size=60 --check-handle-count table = []; -- 2.34.1