From 43289f18cd01f0425d5fc07231f6888859245bf7 Mon Sep 17 00:00:00 2001 From: Georg Reinke Date: Sun, 13 Feb 2022 11:46:51 +0100 Subject: [PATCH] prop: copy initial values and store as pointers internally Without using pointers internally, accepting structs using Store wouldn't work properly. This requires making a copy of the initial values (which were documented already as "initial" values, but were indeed just reused by the existing code). But using them directly was anyway not possible since access not going through Get or GetMust could have led to race conditions anyway. Fixes #298. --- prop/prop.go | 46 +++++++++++++++++++++++++++++++--------------- prop/prop_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/prop/prop.go b/prop/prop.go index a69186b8..2c2a6132 100644 --- a/prop/prop.go +++ b/prop/prop.go @@ -126,7 +126,9 @@ const IntrospectDataString = ` // Prop represents a single property. It is used for creating a Properties // value. type Prop struct { - // Initial value. Must be a DBus-representable type. + // Initial value. Must be a DBus-representable type. This is not modified + // after Properties has been initialized; use Get or GetMust to access the + // value. Value interface{} // If true, the value can be modified by calls to Set. @@ -154,7 +156,7 @@ func (p *Prop) Introspection(name string) introspect.Property { } result.Annotations = []introspect.Annotation{ { - Name: "org.freedesktop.DBus.Property.EmitsChangedSignal", + Name: "org.freedesktop.DBus.Property.EmitsChangedSignal", Value: p.Emit.String(), }, } @@ -173,7 +175,7 @@ type Change struct { // using the org.freedesktop.DBus.Properties interface. It is safe for // concurrent use by multiple goroutines. type Properties struct { - m map[string]map[string]*Prop + m Map mut sync.RWMutex conn *dbus.Conn path dbus.ObjectPath @@ -183,7 +185,7 @@ type Properties struct { // swallowing the error, shouldn't be used. // // Deprecated: use Export instead. -func New(conn *dbus.Conn, path dbus.ObjectPath, props map[string]map[string]*Prop) *Properties { +func New(conn *dbus.Conn, path dbus.ObjectPath, props Map) *Properties { p, err := Export(conn, path, props) if err != nil { return nil @@ -196,15 +198,33 @@ func New(conn *dbus.Conn, path dbus.ObjectPath, props map[string]map[string]*Pro // second-level key is the name of the property. The returned structure will be // exported as org.freedesktop.DBus.Properties on path. func Export( - conn *dbus.Conn, path dbus.ObjectPath, props map[string]map[string]*Prop, + conn *dbus.Conn, path dbus.ObjectPath, props Map, ) (*Properties, error) { - p := &Properties{m: props, conn: conn, path: path} + p := &Properties{m: copyProps(props), conn: conn, path: path} if err := conn.Export(p, path, "org.freedesktop.DBus.Properties"); err != nil { return nil, err } return p, nil } +// Map is a helper type for supplying the configuration of properties to be handled. +type Map = map[string]map[string]*Prop + +func copyProps(in Map) Map { + out := make(Map, len(in)) + for intf, props := range in { + out[intf] = make(map[string]*Prop) + for name, prop := range props { + out[intf][name] = new(Prop) + *out[intf][name] = *prop + val := reflect.New(reflect.TypeOf(prop.Value)) + val.Elem().Set(reflect.ValueOf(prop.Value)) + out[intf][name].Value = val.Interface() + } + } + return out +} + // Get implements org.freedesktop.DBus.Properties.Get. func (p *Properties) Get(iface, property string) (dbus.Variant, *dbus.Error) { p.mut.RLock() @@ -217,7 +237,7 @@ func (p *Properties) Get(iface, property string) (dbus.Variant, *dbus.Error) { if !ok { return dbus.Variant{}, ErrPropNotFound } - return dbus.MakeVariant(prop.Value), nil + return dbus.MakeVariant(reflect.ValueOf(prop.Value).Elem().Interface()), nil } // GetAll implements org.freedesktop.DBus.Properties.GetAll. @@ -230,7 +250,7 @@ func (p *Properties) GetAll(iface string) (map[string]dbus.Variant, *dbus.Error) } rm := make(map[string]dbus.Variant, len(m)) for k, v := range m { - rm[k] = dbus.MakeVariant(v.Value) + rm[k] = dbus.MakeVariant(reflect.ValueOf(v.Value).Elem().Interface()) } return rm, nil } @@ -240,7 +260,7 @@ func (p *Properties) GetAll(iface string) (map[string]dbus.Variant, *dbus.Error) func (p *Properties) GetMust(iface, property string) interface{} { p.mut.RLock() defer p.mut.RUnlock() - return p.m[iface][property].Value + return reflect.ValueOf(p.m[iface][property].Value).Elem().Interface() } // Introspection returns the introspection data that represents the properties @@ -260,9 +280,6 @@ func (p *Properties) Introspection(iface string) []introspect.Property { // must already be locked. func (p *Properties) set(iface, property string, v interface{}) error { prop := p.m[iface][property] - if reflect.ValueOf(prop.Value).Kind() != reflect.Ptr { - prop.Value = reflect.New(reflect.TypeOf(prop.Value)).Interface() - } err := dbus.Store([]interface{}{v}, prop.Value) if err != nil { return err @@ -324,9 +341,8 @@ func (p *Properties) Set(iface, property string, newv dbus.Variant) *dbus.Error func (p *Properties) SetMust(iface, property string, v interface{}) { p.mut.Lock() defer p.mut.Unlock() // unlock in case of panic - prop := p.m[iface][property] - prop.Value = v - if err := p.emitChange(iface, property); err != nil { + err := p.set(iface, property, v) + if err != nil { panic(err) } } diff --git a/prop/prop_test.go b/prop/prop_test.go index b7ee047a..32a20f4b 100644 --- a/prop/prop_test.go +++ b/prop/prop_test.go @@ -100,3 +100,47 @@ func TestValidateStructsAsProp(t *testing.T) { comparePropValue(obj, "FooStructPtr", *zooPtr, t) comparePropValue(obj, "SliceOfFoos", zoos, t) } + +func TestInt32(t *testing.T) { + srv, err := dbus.SessionBus() + if err != nil { + t.Fatal(err) + } + defer srv.Close() + + cli, err := dbus.SessionBus() + if err != nil { + t.Fatal(err) + } + defer cli.Close() + + propsSpec := map[string]map[string]*Prop{ + "org.guelfey.DBus.Test": { + "int32": { + int32(100), + true, + EmitTrue, + nil, + }, + }, + } + props := New(srv, "/org/guelfey/DBus/Test", propsSpec) + + obj := cli.Object(srv.Names()[0], "/org/guelfey/DBus/Test") + + comparePropValue(obj, "int32", int32(100), t) + r := props.GetMust("org.guelfey.DBus.Test", "int32") + if r != int32(100) { + t.Errorf("expected r to be int32(100), but was %#v", r) + } + + if err := props.Set("org.guelfey.DBus.Test", "int32", dbus.MakeVariant(int32(101))); err != nil { + t.Fatalf("failed to set prop int32 to 101") + } + + comparePropValue(obj, "int32", int32(101), t) + r = props.GetMust("org.guelfey.DBus.Test", "int32") + if r != int32(101) { + t.Errorf("expected r to be int32(101), but was %#v", r) + } +}