From f7b24ad541a1502b10d194546125cf9d25bb7a58 Mon Sep 17 00:00:00 2001 From: Bjarne Rosengren Date: Fri, 23 Sep 2022 13:32:24 +0200 Subject: [PATCH 1/4] Protect object map from concurrent access --- default_handler.go | 10 +++++++--- export.go | 13 +++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/default_handler.go b/default_handler.go index cfe8fb26..af1c763d 100644 --- a/default_handler.go +++ b/default_handler.go @@ -38,12 +38,16 @@ type defaultHandler struct { defaultIntf map[string]*exportedIntf } -func (h *defaultHandler) PathExists(path ObjectPath) bool { - _, ok := h.objects[path] - return ok +func (h *defaultHandler) PathExists(path ObjectPath) (*exportedObj, bool) { + h.RLock() + defer h.RUnlock() + obj, ok := h.objects[path] + return obj, ok } func (h *defaultHandler) introspectPath(path ObjectPath) string { + h.RLock() + defer h.RUnlock() subpath := make(map[string]struct{}) var xml bytes.Buffer xml.WriteString("") diff --git a/export.go b/export.go index 34dac9ea..152f37d5 100644 --- a/export.go +++ b/export.go @@ -366,8 +366,7 @@ func (conn *Conn) exportMethodTable(methods map[string]interface{}, path ObjectP } func (conn *Conn) unexport(h *defaultHandler, path ObjectPath, iface string) error { - if h.PathExists(path) { - obj := h.objects[path] + if obj, ok := h.PathExists(path); ok { obj.DeleteInterface(iface) if len(obj.interfaces) == 0 { h.DeleteObject(path) @@ -396,7 +395,7 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac // If this is the first handler for this path, make a new map to hold all // handlers for this path. - if !h.PathExists(path) { + if _, ok := h.PathExists(path); !ok { h.AddObject(path, newExportedObject()) } @@ -406,9 +405,11 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac } // Finally, save this handler - obj := h.objects[path] - obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) - + if obj, ok := h.PathExists(path); ok { + obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) + } else { + return fmt.Errorf(`dbus: Object removed while export: "%s"`, path) + } return nil } From 5951285ccc805a34bffc1e2ad4c9b9c641dcd85d Mon Sep 17 00:00:00 2001 From: Bjarne Rosengren Date: Thu, 10 Nov 2022 14:22:47 +0100 Subject: [PATCH 2/4] Rename PathExists into GetExportedObject --- default_handler.go | 2 +- export.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/default_handler.go b/default_handler.go index af1c763d..b2cbceb2 100644 --- a/default_handler.go +++ b/default_handler.go @@ -38,7 +38,7 @@ type defaultHandler struct { defaultIntf map[string]*exportedIntf } -func (h *defaultHandler) PathExists(path ObjectPath) (*exportedObj, bool) { +func (h *defaultHandler) GetExportedObject(path ObjectPath) (*exportedObj, bool) { h.RLock() defer h.RUnlock() obj, ok := h.objects[path] diff --git a/export.go b/export.go index 152f37d5..85c84be2 100644 --- a/export.go +++ b/export.go @@ -366,7 +366,7 @@ func (conn *Conn) exportMethodTable(methods map[string]interface{}, path ObjectP } func (conn *Conn) unexport(h *defaultHandler, path ObjectPath, iface string) error { - if obj, ok := h.PathExists(path); ok { + if obj, ok := h.GetExportedObject(path); ok { obj.DeleteInterface(iface) if len(obj.interfaces) == 0 { h.DeleteObject(path) @@ -395,7 +395,7 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac // If this is the first handler for this path, make a new map to hold all // handlers for this path. - if _, ok := h.PathExists(path); !ok { + if _, ok := h.GetExportedObject(path); !ok { h.AddObject(path, newExportedObject()) } @@ -405,7 +405,7 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac } // Finally, save this handler - if obj, ok := h.PathExists(path); ok { + if obj, ok := h.GetExportedObject(path); ok { obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) } else { return fmt.Errorf(`dbus: Object removed while export: "%s"`, path) From b4472fc96088d9eacde4072605c7c6079a7b2e9e Mon Sep 17 00:00:00 2001 From: Bjarne Rosengren Date: Tue, 25 Apr 2023 14:34:46 +0200 Subject: [PATCH 3/4] Atomic function to get an exported object for a path GetAnExportedObject returns either an existing object or a newly created object for a specific path. This will solve the case where we could get a mixup when two different goroutines competed to create an object for a specific path. The result might still be unsatifying but atleast its either or and not a mix. --- default_handler.go | 13 +++++++++++++ export.go | 16 +++++----------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/default_handler.go b/default_handler.go index dfcb5a92..2aaeb8dc 100644 --- a/default_handler.go +++ b/default_handler.go @@ -45,6 +45,19 @@ func (h *defaultHandler) GetExportedObject(path ObjectPath) (*exportedObj, bool) return obj, ok } +// GetAnExportedObject returns an exportedObj for an specific ObjectPath +// A new exportedObj is creaated if none existed for ObjectPath +func (h *defaultHandler) GetAnExportedObject(path ObjectPath) *exportedObj { + h.RLock() + defer h.RUnlock() + obj, ok := h.objects[path] + if !ok { + obj = newExportedObject() + h.objects[path] = obj + } + return obj +} + func (h *defaultHandler) introspectPath(path ObjectPath) string { h.RLock() defer h.RUnlock() diff --git a/export.go b/export.go index 548a1094..4433bdd6 100644 --- a/export.go +++ b/export.go @@ -390,23 +390,17 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac return conn.unexport(h, path, iface) } - // If this is the first handler for this path, make a new map to hold all - // handlers for this path. - if _, ok := h.GetExportedObject(path); !ok { - h.AddObject(path, newExportedObject()) - } - + // Get all Methods that should be exported exportedMethods := make(map[string]Method) for name, method := range methods { exportedMethods[name] = exportedMethod{method} } + // Get a handler for the path, either an existing or a new fresh one + obj := h.GetAnExportedObject(path) + // Finally, save this handler - if obj, ok := h.GetExportedObject(path); ok { - obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) - } else { - return fmt.Errorf(`dbus: Object removed while export: "%s"`, path) - } + obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) return nil } From 6d9aa2181561e4d562666669825ae78ba902f2cf Mon Sep 17 00:00:00 2001 From: Bjarne Rosengren Date: Tue, 25 Apr 2023 14:34:46 +0200 Subject: [PATCH 4/4] Atomic function to get an exported object for a path GetOrAddExportedObject returns either an existing object or a newly created object for a specific path. This will solve the case where we could get a mixup when two different goroutines competed to create an object for a specific path. The result might still be unsatifying but atleast its either or and not a mix. --- default_handler.go | 17 +++++++++++++++-- export.go | 16 +++++----------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/default_handler.go b/default_handler.go index dfcb5a92..28dc9e0f 100644 --- a/default_handler.go +++ b/default_handler.go @@ -45,6 +45,19 @@ func (h *defaultHandler) GetExportedObject(path ObjectPath) (*exportedObj, bool) return obj, ok } +// GetOrAddExportedObject returns an exportedObj for an specific ObjectPath +// A new exportedObj is created if none existed for ObjectPath +func (h *defaultHandler) GetOrAddExportedObject(path ObjectPath) *exportedObj { + h.RLock() + defer h.RUnlock() + obj, ok := h.objects[path] + if !ok { + obj = newExportedObject() + h.objects[path] = obj + } + return obj +} + func (h *defaultHandler) introspectPath(path ObjectPath) string { h.RLock() defer h.RUnlock() @@ -69,8 +82,8 @@ func (h *defaultHandler) introspectPath(path ObjectPath) string { } func (h *defaultHandler) LookupObject(path ObjectPath) (ServerObject, bool) { - h.RLock() - defer h.RUnlock() + h.Lock() + defer h.Unlock() object, ok := h.objects[path] if ok { return object, ok diff --git a/export.go b/export.go index 548a1094..4c8a1096 100644 --- a/export.go +++ b/export.go @@ -390,23 +390,17 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac return conn.unexport(h, path, iface) } - // If this is the first handler for this path, make a new map to hold all - // handlers for this path. - if _, ok := h.GetExportedObject(path); !ok { - h.AddObject(path, newExportedObject()) - } - + // Get all Methods that should be exported exportedMethods := make(map[string]Method) for name, method := range methods { exportedMethods[name] = exportedMethod{method} } + // Get a handler for the path, either an existing or a new fresh one + obj := h.GetOrAddExportedObject(path) + // Finally, save this handler - if obj, ok := h.GetExportedObject(path); ok { - obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) - } else { - return fmt.Errorf(`dbus: Object removed while export: "%s"`, path) - } + obj.AddInterface(iface, newExportedIntf(exportedMethods, includeSubtree)) return nil }