Repository: godbus/dbus (go)

Issue:
Prevent encoding errors
As seen in e.g. #244, #240, #205, it's relatively easy for application code to create messages that are ill-formatted semantically, and thus trigger a disconnection by the bus daemon. We should check this directly in library code to prevent one wrong message from terminating the whole connection, which would also result in better error messages:
1. Ensure that message.IsValid covers everything that already is checked in encoder.go
2. Check for valid format for error and signal names
3. Check IsValid in Emit
Method replies are a not-so-nice case because there's no real way to give feedback from the library to the application code here when using Export (without an API break), but just writing an error to stderr (with the option to disable it) would be an alternative.

Pull request:
Improved encoding error handling
Closes #295.

Logging from library code is not nice, but the best way to handle invalid reply messages IMO in a compatible way since there's not really a mechanism to make this known to application code.

Patch:
diff --git a/conn.go b/conn.go
index 1a92b8b..c50bfe3 100644
--- a/conn.go
+++ b/conn.go
@@ -650,9 +650,7 @@ func (conn *Conn) RemoveMatchSignalContext(ctx context.Context, options ...Match
 
 // Signal registers the given channel to be passed all received signal messages.
 //
-// Multiple of these channels can be registered at the same time. The channel is
-// closed if the Conn is closed; it should not be closed by the caller before
-// RemoveSignal was called on it.
+// Multiple of these channels can be registered at the same time.
 //
 // These channels are "overwritten" by Eavesdrop; i.e., if there currently is a
 // channel for eavesdropped messages, this channel receives all signals, and
diff --git a/export.go b/export.go
index 5223347..d3dd9f7 100644
--- a/export.go
+++ b/export.go
@@ -3,6 +3,7 @@ package dbus
 import (
 	"errors"
 	"fmt"
+	"os"
 	"reflect"
 	"strings"
 )
@@ -209,28 +210,23 @@ func (conn *Conn) handleCall(msg *Message) {
 		}
 		reply.Headers[FieldSignature] = MakeVariant(SignatureOf(reply.Body...))
 
-		conn.sendMessageAndIfClosed(reply, nil)
+		if err := reply.IsValid(); err != nil {
+			fmt.Fprintf(os.Stderr, "dbus: dropping invalid reply to %s.%s on obj %s: %s\n", ifaceName, name, path, err)
+		} else {
+			conn.sendMessageAndIfClosed(reply, nil)
+		}
 	}
 }
 
 // Emit emits the given signal on the message bus. The name parameter must be
 // formatted as "interface.member", e.g., "org.freedesktop.DBus.NameLost".
 func (conn *Conn) Emit(path ObjectPath, name string, values ...interface{}) error {
-	if !path.IsValid() {
-		return errors.New("dbus: invalid object path")
-	}
 	i := strings.LastIndex(name, ".")
 	if i == -1 {
 		return errors.New("dbus: invalid method name")
 	}
 	iface := name[:i]
 	member := name[i+1:]
-	if !isValidMember(member) {
-		return errors.New("dbus: invalid method name")
-	}
-	if !isValidInterface(iface) {
-		return errors.New("dbus: invalid interface name")
-	}
 	msg := new(Message)
 	msg.Type = TypeSignal
 	msg.Headers = make(map[HeaderField]Variant)
@@ -241,6 +237,9 @@ func (conn *Conn) Emit(path ObjectPath, name string, values ...interface{}) erro
 	if len(values) > 0 {
 		msg.Headers[FieldSignature] = MakeVariant(SignatureOf(values...))
 	}
+	if err := msg.IsValid(); err != nil {
+		return err
+	}
 
 	var closed bool
 	conn.sendMessageAndIfClosed(msg, func() {
diff --git a/message.go b/message.go
index 16693eb..bdf43fd 100644
--- a/message.go
+++ b/message.go
@@ -208,7 +208,7 @@ func DecodeMessageWithFDs(rd io.Reader, fds []int) (msg *Message, err error) {
 // The possibly returned error can be an error of the underlying reader, an
 // InvalidMessageError or a FormatError.
 func DecodeMessage(rd io.Reader) (msg *Message, err error) {
-	return DecodeMessageWithFDs(rd, make([]int, 0));
+	return DecodeMessageWithFDs(rd, make([]int, 0))
 }
 
 type nullwriter struct{}
@@ -227,8 +227,8 @@ func (msg *Message) CountFds() (int, error) {
 }
 
 func (msg *Message) EncodeToWithFDs(out io.Writer, order binary.ByteOrder) (fds []int, err error) {
-	if err := msg.IsValid(); err != nil {
-		return make([]int, 0), err
+	if err := msg.validateHeader(); err != nil {
+		return nil, err
 	}
 	var vs [7]interface{}
 	switch order {
@@ -237,7 +237,7 @@ func (msg *Message) EncodeToWithFDs(out io.Writer, order binary.ByteOrder) (fds
 	case binary.BigEndian:
 		vs[0] = byte('B')
 	default:
-		return make([]int, 0), errors.New("dbus: invalid byte order")
+		return nil, errors.New("dbus: invalid byte order")
 	}
 	body := new(bytes.Buffer)
 	fds = make([]int, 0)
@@ -284,8 +284,13 @@ func (msg *Message) EncodeTo(out io.Writer, order binary.ByteOrder) (err error)
 }
 
 // IsValid checks whether msg is a valid message and returns an
-// InvalidMessageError if it is not.
+// InvalidMessageError or FormatError if it is not.
 func (msg *Message) IsValid() error {
+	var b bytes.Buffer
+	return msg.EncodeTo(&b, nativeEndian)
+}
+
+func (msg *Message) validateHeader() error {
 	if msg.Flags & ^(FlagNoAutoStart|FlagNoReplyExpected|FlagAllowInteractiveAuthorization) != 0 {
 		return InvalidMessageError("invalid flags")
 	}
@@ -330,6 +335,7 @@ func (msg *Message) IsValid() error {
 			return InvalidMessageError("missing signature")
 		}
 	}
+
 	return nil
 }
 


Test patch:
diff --git a/message_test.go b/message_test.go
new file mode 100644
index 0000000..d6c73da
--- /dev/null
+++ b/message_test.go
@@ -0,0 +1,111 @@
+package dbus
+
+import "testing"
+
+func TestMessage_validateHeader(t *testing.T) {
+	var tcs = []struct {
+		msg Message
+		err error
+	}{
+		{
+			msg: Message{
+				Flags: 0xFF,
+			},
+			err: InvalidMessageError("invalid flags"),
+		},
+		{
+			msg: Message{
+				Type: 0xFF,
+			},
+			err: InvalidMessageError("invalid message type"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+				Headers: map[HeaderField]Variant{
+					0xFF: MakeVariant("foo"),
+				},
+			},
+			err: InvalidMessageError("invalid header"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+				Headers: map[HeaderField]Variant{
+					FieldPath: MakeVariant(42),
+				},
+			},
+			err: InvalidMessageError("invalid type of header field"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+			},
+			err: InvalidMessageError("missing required header"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+				Headers: map[HeaderField]Variant{
+					FieldPath:   MakeVariant(ObjectPath("break")),
+					FieldMember: MakeVariant("foo.bar"),
+				},
+			},
+			err: InvalidMessageError("invalid path name"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+				Headers: map[HeaderField]Variant{
+					FieldPath:   MakeVariant(ObjectPath("/")),
+					FieldMember: MakeVariant("2"),
+				},
+			},
+			err: InvalidMessageError("invalid member name"),
+		},
+		{
+			msg: Message{
+				Type: TypeMethodCall,
+				Headers: map[HeaderField]Variant{
+					FieldPath:      MakeVariant(ObjectPath("/")),
+					FieldMember:    MakeVariant("foo.bar"),
+					FieldInterface: MakeVariant("break"),
+				},
+			},
+			err: InvalidMessageError("invalid interface name"),
+		},
+		{
+			msg: Message{
+				Type: TypeError,
+				Headers: map[HeaderField]Variant{
+					FieldReplySerial: MakeVariant(uint32(0)),
+					FieldErrorName:   MakeVariant("break"),
+				},
+			},
+			err: InvalidMessageError("invalid error name"),
+		},
+		{
+
+			msg: Message{
+				Type: TypeError,
+				Headers: map[HeaderField]Variant{
+					FieldReplySerial: MakeVariant(uint32(0)),
+					FieldErrorName:   MakeVariant("error.name"),
+				},
+				Body: []interface{}{
+					"break",
+				},
+			},
+			err: InvalidMessageError("missing signature"),
+		},
+	}
+
+	for _, tc := range tcs {
+		t.Run(tc.err.Error(), func(t *testing.T) {
+			err := tc.msg.validateHeader()
+			if err != tc.err {
+				t.Errorf("expected error %q, got %q", tc.err, err)
+			}
+		})
+	}
+}