Repository: godbus/dbus
Pull request #363 (2023-04-08 14:31:08)

Omit extra (*Message).IsValid for outgoing messages


Patch:
diff --git a/conn.go b/conn.go
index f9cf27b..bbe111b 100644
--- a/conn.go
+++ b/conn.go
@@ -484,7 +484,7 @@ func (conn *Conn) Object(dest string, path ObjectPath) BusObject {
 	return &Object{conn, dest, path}
 }
 
-func (conn *Conn) sendMessageAndIfClosed(msg *Message, ifClosed func()) {
+func (conn *Conn) sendMessageAndIfClosed(msg *Message, ifClosed func()) error {
 	if msg.serial == 0 {
 		msg.serial = conn.getSerial()
 	}
@@ -497,6 +497,7 @@ func (conn *Conn) sendMessageAndIfClosed(msg *Message, ifClosed func()) {
 	} else if msg.Type != TypeMethodCall {
 		conn.serialGen.RetireSerial(msg.serial)
 	}
+	return err
 }
 
 func (conn *Conn) handleSendError(msg *Message, err error) {
@@ -504,6 +505,9 @@ func (conn *Conn) handleSendError(msg *Message, err error) {
 		conn.calls.handleSendError(msg, err)
 	} else if msg.Type == TypeMethodReply {
 		if _, ok := err.(FormatError); ok {
+			// Make sure that the caller gets some kind of error response if
+			// the application code tried to respond, but the resulting message
+			// was malformed in the end
 			conn.sendError(err, msg.Headers[FieldDestination].value.(string), msg.Headers[FieldReplySerial].value.(uint32))
 		}
 	}
@@ -559,7 +563,8 @@ func (conn *Conn) send(ctx context.Context, msg *Message, ch chan *Call) *Call {
 			<-ctx.Done()
 			conn.calls.handleSendError(msg, ctx.Err())
 		}()
-		conn.sendMessageAndIfClosed(msg, func() {
+		// error is handled in handleSendError
+		_ = conn.sendMessageAndIfClosed(msg, func() {
 			conn.calls.handleSendError(msg, ErrClosed)
 			canceler()
 		})
@@ -567,7 +572,8 @@ func (conn *Conn) send(ctx context.Context, msg *Message, ch chan *Call) *Call {
 		canceler()
 		call = &Call{Err: nil, Done: ch}
 		ch <- call
-		conn.sendMessageAndIfClosed(msg, func() {
+		// error is handled in handleSendError
+		_ = conn.sendMessageAndIfClosed(msg, func() {
 			call = &Call{Err: ErrClosed}
 		})
 	}
@@ -601,7 +607,8 @@ func (conn *Conn) sendError(err error, dest string, serial uint32) {
 	if len(e.Body) > 0 {
 		msg.Headers[FieldSignature] = MakeVariant(SignatureOf(e.Body...))
 	}
-	conn.sendMessageAndIfClosed(msg, nil)
+	// not much we can do to handle a possible error here
+	_ = conn.sendMessageAndIfClosed(msg, nil)
 }
 
 // sendReply creates a method reply message corresponding to the parameters and
@@ -618,7 +625,8 @@ func (conn *Conn) sendReply(dest string, serial uint32, values ...interface{}) {
 	if len(values) > 0 {
 		msg.Headers[FieldSignature] = MakeVariant(SignatureOf(values...))
 	}
-	conn.sendMessageAndIfClosed(msg, nil)
+	// not much we can do to handle a possible error here
+	_ = conn.sendMessageAndIfClosed(msg, nil)
 }
 
 // AddMatchSignal registers the given match rule to receive broadcast
diff --git a/export.go b/export.go
index 34dac9e..fa009d2 100644
--- a/export.go
+++ b/export.go
@@ -208,10 +208,10 @@ func (conn *Conn) handleCall(msg *Message) {
 		copy(reply.Body, ret)
 		reply.Headers[FieldSignature] = MakeVariant(SignatureOf(reply.Body...))
 
-		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)
+		if err := conn.sendMessageAndIfClosed(reply, nil); err != nil {
+			if _, ok := err.(FormatError); ok {
+				fmt.Fprintf(os.Stderr, "dbus: replacing invalid reply to %s.%s on obj %s: %s\n", ifaceName, name, path, err)
+			}
 		}
 	}
 }
@@ -235,18 +235,15 @@ 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() {
+	err := conn.sendMessageAndIfClosed(msg, func() {
 		closed = true
 	})
 	if closed {
 		return ErrClosed
 	}
-	return nil
+	return err
 }
 
 // Export registers the given value to be exported as an object on the


Test patch:
diff --git a/conn_test.go b/conn_test.go
index a97f6ca..9f64eca 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -497,7 +497,7 @@ process:
 				reply.Body = make([]interface{}, 1)
 				reply.Body[0] = state
 				reply.Headers[FieldSignature] = MakeVariant(SignatureOf(reply.Body...))
-				srv.sendMessageAndIfClosed(reply, nil)
+				_ = srv.sendMessageAndIfClosed(reply, nil)
 			}
 		case <-ctx.Done():
 			t.Logf("Context cancelled, server emitted %v signals", state)
diff --git a/export_test.go b/export_test.go
index a1c1b8b..5d04575 100644
--- a/export_test.go
+++ b/export_test.go
@@ -30,6 +30,12 @@ func (export badExport) Foo(param string) string {
 	return "bar"
 }
 
+type invalidMessageExport struct{}
+
+func (export invalidMessageExport) Foo(param string) (string, *Error) {
+	return "\x00", nil
+}
+
 type errorExport struct {
 	message Message
 }
@@ -48,6 +54,20 @@ func (export *noErrorExport) Run(message Message, param string) string {
 	return "cool"
 }
 
+// Test that trying to emit an invalid message leads to an error.
+func TestEmit_invalidMessage(t *testing.T) {
+	connection, err := ConnectSessionBus()
+	if err != nil {
+		t.Fatalf("Unexpected error connecting to session bus: %s", err)
+	}
+	defer connection.Close()
+
+	err = connection.Emit("/org/guelfey/DBus/Test", "org.guelfey.DBusTest", "\x00")
+	if _, ok := err.(FormatError); !ok {
+		t.Fatal("expected FormatError when emitting invalid message")
+	}
+}
+
 // Test typical Export usage.
 func TestExport(t *testing.T) {
 	connection, err := ConnectSessionBus()
@@ -254,6 +274,32 @@ func TestExport_badSignature(t *testing.T) {
 	}
 }
 
+// Test Export with a method returning an invalid message. This should result in an
+// error reply being generated instead.
+func TestExport_invalidMessage(t *testing.T) {
+	connection, err := ConnectSessionBus()
+	if err != nil {
+		t.Fatalf("Unexpected error connecting to session bus: %s", err)
+	}
+	defer connection.Close()
+
+	name := connection.Names()[0]
+
+	err = connection.Export(invalidMessageExport{}, "/org/guelfey/DBus/Test", "org.guelfey.DBus.Test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	object := connection.Object(name, "/org/guelfey/DBus/Test")
+
+	var response string
+	call := object.Call("org.guelfey.DBus.Test.Foo", 0, "test")
+	err = call.Store(&response)
+	t.Log(err)
+	if err == nil {
+		t.Errorf("Expected an error due to the response message being invalid")
+	}
+}
+
 // Test typical ExportWithMap usage.
 func TestExportWithMap(t *testing.T) {
 	connection, err := ConnectSessionBus()