From 8fa0f56f453a1e22406f0e057be27787bf49b3be Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Wed, 28 Aug 2024 16:41:50 +0000 Subject: [PATCH] [VM/Service] Add private _deleteIdZone RPC TEST=pkg/vm_service/test/id_zones_test.dart, CI Issue: https://github.com/dart-lang/sdk/issues/55869 Change-Id: I0b951505edd98364373d5913b7a01f6d4775998e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380360 Commit-Queue: Derek Xu Reviewed-by: Ben Konyi --- pkg/vm_service/test/id_zones_test.dart | 41 +++++++++ runtime/vm/isolate.cc | 7 ++ runtime/vm/isolate.h | 2 + runtime/vm/service.cc | 114 +++++++++++++++++-------- runtime/vm/service.h | 7 ++ 5 files changed, 135 insertions(+), 36 deletions(-) diff --git a/pkg/vm_service/test/id_zones_test.dart b/pkg/vm_service/test/id_zones_test.dart index 708949175bc..ffccbb6d13d 100644 --- a/pkg/vm_service/test/id_zones_test.dart +++ b/pkg/vm_service/test/id_zones_test.dart @@ -263,6 +263,47 @@ final tests = [ expect(cInstanceRef3['type'], '@Instance'); expect(cInstanceRef3['id'], 'objects/0/3'); }, + + // Test deleting an ID Zone. + (VmService service, IsolateRef isolateRef) async { + final isolateId = isolateRef.id!; + final idZone4 = (await service.callMethod( + '_createIdZone', + isolateId: isolateId, + args: { + 'backingBufferKind': 'Ring', + 'idAssignmentPolicy': 'AlwaysAllocate', + }, + )) + .json!; + expect(idZone4['type'], '_IdZone'); + expect(idZone4['id'], 'zones/4'); + expect(idZone4['backingBufferKind'], 'Ring'); + expect(idZone4['idAssignmentPolicy'], 'AlwaysAllocate'); + + await service.callMethod( + '_deleteIdZone', + isolateId: isolateId, + args: { + '_idZoneId': idZone4['id'], + }, + ); + + try { + await service.callMethod( + 'evaluateInFrame', + isolateId: isolateId, + args: { + 'frameIndex': 0, + 'expression': 'c', + '_idZoneId': idZone4['id'], + }, + ); + fail('successfully used an ID zone that should have been deleted'); + } on RPCError catch (e) { + expect(e.code, RPCErrorKind.kInvalidParams.code); + } + }, resumeIsolate, ]; diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 93ee220909a..792bfa6191f 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -3023,6 +3023,13 @@ RingServiceIdZone& Isolate::AddServiceIdZone( } } +void Isolate::DeleteServiceIdZone(int32_t id) { + ASSERT(service_id_zones_ != nullptr); + ASSERT(id < service_id_zones_->length()); + delete service_id_zones_->At(id); + (*service_id_zones_)[id] = nullptr; +} + RingServiceIdZone& Isolate::EnsureDefaultServiceIdZone() { if (service_id_zones_ == nullptr) { service_id_zones_ = new MallocGrowableArray(); diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 1be04667704..a1d43d670a9 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -1256,6 +1256,8 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry { ObjectIdRing::IdPolicy id_assignment_policy, int32_t capacity); + void DeleteServiceIdZone(int32_t id); + // The default Service ID zone is created lazily; this method returns the // default Service ID zone, creating it if necessary. RingServiceIdZone& EnsureDefaultServiceIdZone(); diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index 174eacb4b9e..bfdf57ec038 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -185,6 +185,38 @@ class NoSuchParameter : public MethodParameter { #define RUNNABLE_ISOLATE_PARAMETER new RunnableIsolateParameter("isolateId") #define OBJECT_PARAMETER new IdParameter("objectId", true) +static bool ValidateUIntParameter(const char* value) { + if (value == nullptr) { + return false; + } + for (const char* cp = value; *cp != '\0'; cp++) { + if (*cp < '0' || *cp > '9') { + return false; + } + } + return true; +} + +class UIntParameter : public MethodParameter { + public: + UIntParameter(const char* name, bool required) + : MethodParameter(name, required) {} + + virtual bool Validate(const char* value) const { + return ValidateUIntParameter(value); + } + + static uintptr_t Parse(const char* value) { + if (value == nullptr) { + return -1; + } + char* end_ptr = nullptr; + uintptr_t result = strtoul(value, &end_ptr, 10); + ASSERT(*end_ptr == '\0'); // Parsed full string + return result; + } +}; + class EnumListParameter : public MethodParameter { public: EnumListParameter(const char* name, bool required, const char* const* enums) @@ -370,6 +402,14 @@ ServiceIdZone::ServiceIdZone(intptr_t id, ObjectIdRing::IdPolicy policy) ServiceIdZone::~ServiceIdZone() {} +intptr_t ServiceIdZone::StringIdToInt(const char* id_string) { + if (Utils::StrStartsWith(id_string, "zones/") && + ValidateUIntParameter(id_string + 6)) { + return UIntParameter::Parse(id_string + 6); + } + return -1; +} + RingServiceIdZone::RingServiceIdZone(intptr_t id, ObjectIdRing::IdPolicy policy, int32_t capacity) @@ -751,34 +791,6 @@ class BoolParameter : public MethodParameter { } }; -class UIntParameter : public MethodParameter { - public: - UIntParameter(const char* name, bool required) - : MethodParameter(name, required) {} - - virtual bool Validate(const char* value) const { - if (value == nullptr) { - return false; - } - for (const char* cp = value; *cp != '\0'; cp++) { - if (*cp < '0' || *cp > '9') { - return false; - } - } - return true; - } - - static uintptr_t Parse(const char* value) { - if (value == nullptr) { - return -1; - } - char* end_ptr = nullptr; - uintptr_t result = strtoul(value, &end_ptr, 10); - ASSERT(*end_ptr == '\0'); // Parsed full string - return result; - } -}; - class Int64Parameter : public MethodParameter { public: Int64Parameter(const char* name, bool required) @@ -1030,14 +1042,11 @@ ErrorPtr Service::InvokeMethod(Isolate* I, // are about to create, meaning that it is where temporary Service IDs may // be allocated by the RPC currently being handled. RingServiceIdZone* id_zone = &isolate.EnsureDefaultServiceIdZone(); - const char* id_zone_arg = js.LookupParam("_idZoneId"); - if (id_zone_arg != nullptr) { - // We must create a temporary |UIntParameter| to use the - // |UIntParameter::Validate| helper because it is not a static method. - if (Utils::StrStartsWith(id_zone_arg, "zones/") && - UIntParameter("temp", true).Validate(id_zone_arg + 6)) { - id_zone = - isolate.GetServiceIdZone(UIntParameter::Parse(id_zone_arg + 6)); + const char* id_zone_id_arg = js.LookupParam("_idZoneId"); + if (id_zone_id_arg != nullptr) { + intptr_t id_zone_id = ServiceIdZone::StringIdToInt(id_zone_id_arg); + if (id_zone_id != -1) { + id_zone = isolate.GetServiceIdZone(id_zone_id); } else { id_zone = nullptr; } @@ -5182,6 +5191,37 @@ static void CreateIdZone(Thread* thread, JSONStream* js) { .PrintJSON(*js); } +static const MethodParameter* const delete_id_zone_params[] = { + RUNNABLE_ISOLATE_PARAMETER, + nullptr, +}; + +static void DeleteIdZone(Thread* thread, JSONStream* js) { + ASSERT(thread != nullptr); + ASSERT(js != nullptr); + + Isolate* isolate = thread->isolate(); + ASSERT(isolate != nullptr); + + const char* id_zone_id_arg = js->LookupParam("_idZoneId"); + if (id_zone_id_arg == nullptr) { + PrintMissingParamError(js, "_idZoneId"); + } + + // If the `_idZoneId` argument is not missing, we know that the some + // properties of |id_zone_id| have already been checked in |InvokeMethod| + // (search for `js.set_id_zone(*id_zone)` to find the checks), so we can + // assert these properties to be true below. + + intptr_t id_zone_id = ServiceIdZone::StringIdToInt(id_zone_id_arg); + ASSERT(id_zone_id != -1); + ASSERT(id_zone_id < isolate->NumServiceIdZones()); + + isolate->DeleteServiceIdZone(id_zone_id); + + PrintSuccess(js); +} + static const MethodParameter* const get_object_params[] = { RUNNABLE_ISOLATE_PARAMETER, new UIntParameter("offset", false), @@ -6016,6 +6056,8 @@ static const ServiceMethodDescriptor service_methods_[] = { { "_compileExpression", CompileExpression, compile_expression_params }, { "_createIdZone", CreateIdZone, create_id_zone_params }, + { "_deleteIdZone", DeleteIdZone, + delete_id_zone_params }, { "_enableProfiler", EnableProfiler, enable_profiler_params, }, { "evaluate", Evaluate, diff --git a/runtime/vm/service.h b/runtime/vm/service.h index 1eddf42db17..adfd1ff5b98 100644 --- a/runtime/vm/service.h +++ b/runtime/vm/service.h @@ -40,6 +40,13 @@ class ServiceIdZone { ServiceIdZone(intptr_t id, ObjectIdRing::IdPolicy policy); virtual ~ServiceIdZone(); + // Parses a Service ID zone ID string and returns the corresponding integer + // ID. Or, returns -1 if |id_string| is invalid. + // + // For example, this function will return 5 when called with the argument + // "zones/5". + intptr_t static StringIdToInt(const char* id_string); + intptr_t id() const { return id_; } ObjectIdRing::IdPolicy policy() const { return policy_; }