Created attachment 347917 [details] [review]
gdbusmessage: Add missing G_GNUC_PRINTF attribute

This highlighted a bug in GDBusConnection, where an interface name was
not included in a message referring to it.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 347918 [details] [review]
gsubprocess: Add missing G_GNUC_NULL_TERMINATED attribute

g_subprocess_launcher_spawn() is NULL-terminated, and must have a
non-NULL argv0 specified, so G_GNUC_NULL_TERMINATED is appropriate here.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Inspired by heftig on IRC.

Review of attachment 347917 [details] [review]:

ACK.

Review of attachment 347918 [details] [review]:

ACK.

Attachment 347917 [details] pushed as ff327ba - gdbusmessage: Add missing G_GNUC_PRINTF attribute
Attachment 347918 [details] pushed as e1f362b - gsubprocess: Add missing G_GNUC_NULL_TERMINATED attribute

Using G_GNUC_PRINTF(3, 4) here is wrong and should be G_GNUC_PRINTF(3, 0)

(In reply to Rico Tzschichholz from comment #7)
> Using G_GNUC_PRINTF(3, 4) here is wrong and should be G_GNUC_PRINTF(3, 0)

Why? The printf arguments are available to check in parameter 4.

My impression is that g_dbus_message_new_method_error can used with omitted variable argument-list, meaning to just pass a literal string without any format-args. I guess this is not the case here?

For reference this breaks the usage of g_dbus_message_new_method_error in networkmanager https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/nm-manager.c?h=nm-1-4#n4901

G_GNUC_PRINTF(3, 4) should be fine for the case where the third argument doesn’t contain any format placeholders. It’s exactly the same as calling g_strdup_printf ("string with no placeholders").

What’s the warning message you get when compiling NetworkManager?

Its fails to build when using -Werror=format-security

nm-manager.c:4989:44: error: format not a string literal and no format arguments [-Werror=format-security]
                                            (error_message = "Not authorized to perform this operation"));
                                            ^

This is the code:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/nm-manager.c#n5191

if (error || (result != NM_AUTH_CALL_RESULT_YES)) {
  reply = g_dbus_message_new_method_error (pfd->message,
                                           NM_PERM_DENIED_ERROR,
                                           (error_message = "Not authorized to perform this operation"));
  if (error)
    error_message = error->message;
  goto done;
}

Looks to me like gcc is not clever enough to use the string literal from the assignment to error_message. I’d consider that a NetworkManager bug (or a bug in gcc). You can probably fix it as:

if (error || (result != NM_AUTH_CALL_RESULT_YES)) {
  reply = g_dbus_message_new_method_error (pfd->message,
                                           NM_PERM_DENIED_ERROR,
                                           "Not authorized to perform this operation");
  if (error)
    error_message = error->message;
  else
    g_variant_get (g_dbus_message_get_body (reply), "(&s)", &error_message);

  goto done;
}