diff mbox series

[07/11] sockets: Fix default of UnixSocketAddress member @tight

Message ID 20201029133833.3450220-8-armbru@redhat.com
State New, archived
Headers show
Series sockets: Attempt to drain the abstract socket swamp | expand

Commit Message

Markus Armbruster Oct. 29, 2020, 1:38 p.m. UTC
QMP chardev-add defaults absent member @tight to false instead of
true.  HMP chardev-add and CLI -chardev correctly default to true.

The previous commit demonstrated that socket_listen() and
socket_connect() are broken for absent @tight.  That explains why QMP
is broken, but not why HMP and CLI work.  We need to dig deeper.

An optional bool member of a QAPI struct can be false, true, or
absent.  In C, we have:

	    has_MEMBER    MEMBER
    false         true	   false
    true	  true	   false
    absent	 false	false/ignore

When has_MEMBER is false, MEMBER should be set to false on write, and
ignored on read.

unix_listen_saddr() and unix_connect_saddr() use member @tight without
checking @has_tight.  This is wrong.

When @tight was set to false as it should be, absent @tight defaults
to false.  Wrong, it should default to true.  This is what breaks QMP.

There is one exception: qemu_chr_parse_socket() leaves @has_tight
false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
why HMP and CLI work.  Same for @has_abstract.

Fix unix_listen_saddr() and unix_connect_saddr() to default absent
@tight to true.

Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 chardev/char-socket.c     | 2 ++
 tests/test-util-sockets.c | 6 +++---
 util/qemu-sockets.c       | 4 ++--
 3 files changed, 7 insertions(+), 5 deletions(-)

Comments

Paolo Bonzini Oct. 29, 2020, 5:39 p.m. UTC | #1
On 29/10/20 14:38, Markus Armbruster wrote:
> When @tight was set to false as it should be, absent @tight defaults
> to false.  Wrong, it should default to true.  This is what breaks QMP.

When @has_tight...

Paolo
Paolo Bonzini Oct. 29, 2020, 6:05 p.m. UTC | #2
On 29/10/20 18:39, Paolo Bonzini wrote:
>> When @tight was set to false as it should be, absent @tight defaults
>> to false.  Wrong, it should default to true.  This is what breaks QMP.
> When @has_tight...

Ah, I see what you meant here.  Suggested reword:

---------
An optional bool member of a QAPI struct can be false, true, or absent.
The previous commit demonstrated that socket_listen() and
socket_connect() are broken for absent @tight, and indeed QMP chardev-
add also defaults absent member @tight to false instead of true.

In C, QAPI members are represented by two fields, has_MEMBER and MEMBER.
We have:

	    has_MEMBER    MEMBER
    false         true	   false
    true	  true	   false
    absent	 false	false/ignore

When has_MEMBER is false, MEMBER should be set to false on write, and
ignored on read.

For QMP, the QAPI visitors handle absent @tight by setting both
@has_tight and @tight to false.  unix_listen_saddr() and
unix_connect_saddr() however use @tight only, disregarding @has_tight.
This is wrong and means that absent @tight defaults to false whereas it
should default to true.

The same is true for @has_abstract, though @abstract defaults to
false and therefore has the same behavior for all of QMP, HMP and CLI.
Fix unix_listen_saddr() and unix_connect_saddr() to check
@has_abstract/@has_tight, and to default absent @tight to true.

However, this is only half of the story.  HMP chardev-add and CLI
-chardev so far correctly defaulted @tight to true, but defaults to
false again with the above fix for HMP and CLI.  In fact, the "tight"
and "abstract" options now break completely.

Digging deeper, we find that qemu_chr_parse_socket() also ignores
@has_tight, leaving it false when it sets @tight.  That is also wrong,
but the two wrongs cancelled out.  Fix qemu_chr_parse_socket() to set
@has_tight and @has_abstract; writing testcases for HMP and CLI is left
for another day.
---------

Apologies if the last sentence is incorrect. :)

Thanks,

Paolo
Eric Blake Oct. 29, 2020, 7:34 p.m. UTC | #3
On 10/29/20 8:38 AM, Markus Armbruster wrote:
> QMP chardev-add defaults absent member @tight to false instead of
> true.  HMP chardev-add and CLI -chardev correctly default to true.
> 
> The previous commit demonstrated that socket_listen() and
> socket_connect() are broken for absent @tight.  That explains why QMP
> is broken, but not why HMP and CLI work.  We need to dig deeper.
> 
> An optional bool member of a QAPI struct can be false, true, or
> absent.  In C, we have:
> 
> 	    has_MEMBER    MEMBER
>     false         true	   false
>     true	  true	   false
>     absent	 false	false/ignore

I'm not sure the TAB in this table made it very legible (it's hard to
tell if has_MEMBER is the label of column 1 or 2).

Row two is wrong: MEMBER (column 3) is set to true when the QMP code
passed true on the wire.

> 
> When has_MEMBER is false, MEMBER should be set to false on write, and
> ignored on read.
> 
> unix_listen_saddr() and unix_connect_saddr() use member @tight without
> checking @has_tight.  This is wrong.

It generally works if addr was constructed by the same way as the
generated QAPI parser code - but as you demonstrated, in this particular
case, because our construction did not obey the rules of the QAPI
parser, our lack of checking bit us.

> 
> When @tight was set to false as it should be, absent @tight defaults
> to false.  Wrong, it should default to true.  This is what breaks QMP.
> 
> There is one exception: qemu_chr_parse_socket() leaves @has_tight
> false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
> why HMP and CLI work.  Same for @has_abstract.
> 
> Fix unix_listen_saddr() and unix_connect_saddr() to default absent
> @tight to true.
> 
> Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.

At any rate, the fix looks correct:
- as producers, anywhere we hand-construct an addr (rather than using
generated QAPI code), we MUST set both has_MEMBER and MEMBER, including
setting MEMBER to false if has_MEMBER is false, if we want to preserve
the assumptions made in the rest of the code;
- as consumers, rather than relying on the QAPI parsers only setting
MEMBER to true when has_MEMBER is true, we can ensure that has_MEMBER
has priority by checking it ourselves

> +++ b/util/qemu-sockets.c
> @@ -919,7 +919,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>      if (saddr->abstract) {
>          un.sun_path[0] = '\0';
>          memcpy(&un.sun_path[1], path, pathlen);
> -        if (saddr->tight) {
> +        if (!saddr->has_tight || saddr->tight) {
>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>          }
>      } else {
> @@ -979,7 +979,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>      if (saddr->abstract) {
>          un.sun_path[0] = '\0';
>          memcpy(&un.sun_path[1], saddr->path, pathlen);
> -        if (saddr->tight) {
> +        if (!saddr->has_tight || saddr->tight) {
>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>          }
>      } else {
> 

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Oct. 30, 2020, 6:54 a.m. UTC | #4
Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 AM, Markus Armbruster wrote:
>> QMP chardev-add defaults absent member @tight to false instead of
>> true.  HMP chardev-add and CLI -chardev correctly default to true.
>> 
>> The previous commit demonstrated that socket_listen() and
>> socket_connect() are broken for absent @tight.  That explains why QMP
>> is broken, but not why HMP and CLI work.  We need to dig deeper.
>> 
>> An optional bool member of a QAPI struct can be false, true, or
>> absent.  In C, we have:
>> 
>> 	    has_MEMBER    MEMBER
>>     false         true	   false
>>     true	  true	   false
>>     absent	 false	false/ignore
>
> I'm not sure the TAB in this table made it very legible (it's hard to
> tell if has_MEMBER is the label of column 1 or 2).

Use of TAB is an accident.

> Row two is wrong: MEMBER (column 3) is set to true when the QMP code
> passed true on the wire.

Pasto, fixing...

Result:

            has_MEMBER    MEMBER
    false         true     false
    true          true      true
    absent       false  false/ignore

>> When has_MEMBER is false, MEMBER should be set to false on write, and
>> ignored on read.
>> 
>> unix_listen_saddr() and unix_connect_saddr() use member @tight without
>> checking @has_tight.  This is wrong.
>
> It generally works if addr was constructed by the same way as the
> generated QAPI parser code - but as you demonstrated, in this particular
> case, because our construction did not obey the rules of the QAPI
> parser, our lack of checking bit us.
>
>> When @tight was set to false as it should be, absent @tight defaults
>> to false.  Wrong, it should default to true.  This is what breaks QMP.
>> 
>> There is one exception: qemu_chr_parse_socket() leaves @has_tight
>> false when it sets @tight.  Wrong, but the wrongs cancel out.  This is
>> why HMP and CLI work.  Same for @has_abstract.
>> 
>> Fix unix_listen_saddr() and unix_connect_saddr() to default absent
>> @tight to true.
>> 
>> Fix qemu_chr_parse_socket() to set @has_tight and @has_abstract.
>
> At any rate, the fix looks correct:
> - as producers, anywhere we hand-construct an addr (rather than using
> generated QAPI code), we MUST set both has_MEMBER and MEMBER, including
> setting MEMBER to false if has_MEMBER is false, if we want to preserve
> the assumptions made in the rest of the code;
> - as consumers, rather than relying on the QAPI parsers only setting
> MEMBER to true when has_MEMBER is true, we can ensure that has_MEMBER
> has priority by checking it ourselves

As long as the instance is built according to the rules, you can
contract has_MEMBER && MEMBER to just MEMBER.  Both mean "true".

However, has_MEMBER && !MEMBER cannot be contracted to !MEMBER.  The
former means "false", the latter means "false or absent".

Doubters, see the table above.

Putting defaults in the schema would let us eliminate the "absent"
state, the has_MEMBER flags, and the bugs that come with them.  Sadly,
this project has been crowded out by more urgent or important work since
forever.

>> +++ b/util/qemu-sockets.c
>> @@ -919,7 +919,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>>      if (saddr->abstract) {
>>          un.sun_path[0] = '\0';
>>          memcpy(&un.sun_path[1], path, pathlen);
>> -        if (saddr->tight) {
>> +        if (!saddr->has_tight || saddr->tight) {
>>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>>          }
>>      } else {
>> @@ -979,7 +979,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>>      if (saddr->abstract) {
>>          un.sun_path[0] = '\0';
>>          memcpy(&un.sun_path[1], saddr->path, pathlen);
>> -        if (saddr->tight) {
>> +        if (!saddr->has_tight || saddr->tight) {
>>              addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
>>          }
>>      } else {
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
Markus Armbruster Oct. 30, 2020, 6:58 a.m. UTC | #5
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/10/20 18:39, Paolo Bonzini wrote:
>>> When @tight was set to false as it should be, absent @tight defaults
>>> to false.  Wrong, it should default to true.  This is what breaks QMP.
>> When @has_tight...
>
> Ah, I see what you meant here.  Suggested reword:
>
> ---------
> An optional bool member of a QAPI struct can be false, true, or absent.
> The previous commit demonstrated that socket_listen() and
> socket_connect() are broken for absent @tight, and indeed QMP chardev-
> add also defaults absent member @tight to false instead of true.
>
> In C, QAPI members are represented by two fields, has_MEMBER and MEMBER.
> We have:
>
> 	    has_MEMBER    MEMBER
>     false         true	   false
>     true	  true	   false
>     absent	 false	false/ignore
>
> When has_MEMBER is false, MEMBER should be set to false on write, and
> ignored on read.
>
> For QMP, the QAPI visitors handle absent @tight by setting both
> @has_tight and @tight to false.  unix_listen_saddr() and
> unix_connect_saddr() however use @tight only, disregarding @has_tight.
> This is wrong and means that absent @tight defaults to false whereas it
> should default to true.
>
> The same is true for @has_abstract, though @abstract defaults to
> false and therefore has the same behavior for all of QMP, HMP and CLI.
> Fix unix_listen_saddr() and unix_connect_saddr() to check
> @has_abstract/@has_tight, and to default absent @tight to true.
>
> However, this is only half of the story.  HMP chardev-add and CLI
> -chardev so far correctly defaulted @tight to true, but defaults to
> false again with the above fix for HMP and CLI.  In fact, the "tight"
> and "abstract" options now break completely.
>
> Digging deeper, we find that qemu_chr_parse_socket() also ignores
> @has_tight, leaving it false when it sets @tight.  That is also wrong,
> but the two wrongs cancelled out.  Fix qemu_chr_parse_socket() to set
> @has_tight and @has_abstract; writing testcases for HMP and CLI is left
> for another day.
> ---------
>
> Apologies if the last sentence is incorrect. :)

Sold (with the table fixed as per Eric's review)!
diff mbox series

Patch

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 95e45812d5..1ee5a8c295 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1439,7 +1439,9 @@  static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
         q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
         q_unix->path = g_strdup(path);
+        q_unix->has_tight = true;
         q_unix->tight = tight;
+        q_unix->has_abstract = true;
         q_unix->abstract = abstract;
     } else if (host) {
         addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index f8b6586e70..7ecf95579b 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -294,13 +294,13 @@  static void test_socket_unix_abstract(void)
     abstract_socket_matrix_row matrix[ABSTRACT_SOCKET_VARIANTS] = {
         { &addr,
           { &addr_tight, &addr_padded, &addr },
-          { false /* BUG */, true /* BUG */, true } },
+          { true, false, true } },
         { &addr_tight,
           { &addr_padded, &addr, &addr_tight },
-          { false, false /* BUG */, true } },
+          { false, true, true } },
         { &addr_padded,
           { &addr, &addr_tight, &addr_padded },
-          { true /* BUG */, false, true } }
+          { false, false, true } }
     };
     int i;
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 05e5c73f9d..c802d5aa0a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -919,7 +919,7 @@  static int unix_listen_saddr(UnixSocketAddress *saddr,
     if (saddr->abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], path, pathlen);
-        if (saddr->tight) {
+        if (!saddr->has_tight || saddr->tight) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {
@@ -979,7 +979,7 @@  static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
     if (saddr->abstract) {
         un.sun_path[0] = '\0';
         memcpy(&un.sun_path[1], saddr->path, pathlen);
-        if (saddr->tight) {
+        if (!saddr->has_tight || saddr->tight) {
             addrlen = offsetof(struct sockaddr_un, sun_path) + 1 + pathlen;
         }
     } else {