Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect object map from concurrent access #340

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Protect object map from concurrent access #340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7b24ad
Protect object map from concurrent access
ubkr Sep 23, 2022
5951285
Rename PathExists into GetExportedObject
ubkr Nov 10, 2022
17e6ce7
Merge branch 'godbus:master' into master
ubkr Apr 25, 2023
b4472fc
Atomic function to get an exported object for a path
ubkr Apr 25, 2023
6d9aa21
Atomic function to get an exported object for a path
ubkr Apr 25, 2023
3da7206
Merge branch 'master' of https://github.com/ubkr/dbus
ubkr May 30, 2023
0af72af
Merge branch 'godbus:master' into master
ubkr May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions default_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
ubkr marked this conversation as resolved.
Show resolved Hide resolved
h.RLock()
defer h.RUnlock()
obj, ok := h.objects[path]
return obj, ok
Comment on lines +44 to +45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
obj, ok := h.objects[path]
return obj, ok
return h.objects[path]

}

func (h *defaultHandler) introspectPath(path ObjectPath) string {
h.RLock()
defer h.RUnlock()
subpath := make(map[string]struct{})
var xml bytes.Buffer
xml.WriteString("<node>")
Expand Down
13 changes: 7 additions & 6 deletions export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better get a write lock here then and keep it until the end of the function - this also removes the need for checking the other PathExists in L408.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem I can see with that is what lock should be taken, I would argue that it is not a good design to call h.Lock() from this context and if we choose to do that we need to remove the lock call in h.AddObject too, making it asymmetric to h.RemoveObject. To me it is better to let defaultHandler and its functions handle its own locks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the way this is implemented right now introduces a race condition. If you have two export calls running at the same time for the same path, it could happen that:

  1. Call 1 reaches export.go:398. There's no object on that path yet, so ok is false. After GetExportedObject returns, the lock is not held.
  2. Call 2 goes through the whole function, working on the newly created entry in h.objects.
  3. Call 1 is scheduled again and reaches export.go:399, overwriting the entry in h.objects and thus discarding the effect that Call 2 had.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry about the delay..)
@guelfey, Added a new function to defaultHandler, GetAnExportedObject(path). This function will return either an existing object for the path or register a newly created object on the path and return it. It should address your concerns above but I'm not sure that it is a good name though.
Note, I've yet to run it through our own use cases to make sure it works....

h.AddObject(path, newExportedObject())
}

Expand All @@ -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
}

Expand Down