| Message ID | 20200423035640.29202-1-zxq_yx_007@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] qemu-sockets: add abstract UNIX domain socket support | expand |
Adding Eric & Markus for QAPI modelling questions On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote: > unix_connect_saddr now support abstract address type > > By default qemu does not support abstract UNIX domain > socket address. Add this ability to make qemu handy > when abstract address is needed. Was that a specific app you're using with QEMU that needs this ? > Abstract address is marked by prefixing the address name with a '@'. For full support of the abstract namespace we would ned to allow the "sun_path" to contain an arbitrary mix of NULs and non-NULs characters, and allow connect() @addrlen to be an arbitrary size. This patch only allows a single initial NUL, and reqiures @addrlen to be the full size of sun_path, padding with trailing NULs. This limitation is impossible to lift with QEMU's current approach to UNIX sockets, as it relies on passing around a NULL terminated string, so there's no way to have embedded NULs. Since there's no explicit length, we have to chooose between forcing the full sun_path size as @addrlen, or forcing the string length as the @addrlen value. IIUC, socat makes the latter decision by default, but has a flag to switch to the former. [man socat] unix-tightsocklen=[0|1] On socket operations, pass a socket address length that does not include the whole struct sockaddr_un record but (besides other compo‐ nents) only the relevant part of the filename or abstract string. Default is 1. [/man] This actually is supported for both abstract and non-abstract sockets, though IIUC this doesn't make a semantic difference for non-abstract sockets. The point is we have four possible combinations NON-ABSTRACT + FULL SIZE NON-ABSTRACT + MINIMAL SIZE (default) ABSTRACT + FULL SIZE ABSTRACT + MINIMAL SIZE (default) With your patch doing the latter, it means QEMU supports only two combinations NON+ABSTRACT + FULL SIZE ABSTRACT + MINIMAL SIZE and also can't use "@somerealpath" for a non-abstract socket, though admittedly this is unlikely. Socat uses a special option to request use of abstract sockets. eg ABSTRACT:somepath, and automatically adds the leading NUL, so there's no need for a special "@" character. This means that UNIX:@somepath still resolves to a filesystem path and not a abstract socket path. Finally, the patch as only added support for connect() not listen(). I think if QEMU wants to support abstract sockets we must do both, and also have unit tests added to tests/test-util-sockets.c The question is whether we're ok with this simple approach in QEMU, or should do a full approach with more explicit modelling. ie should we change QAPI thus: { 'struct': 'UnixSocketAddress', 'data': { 'path': 'str', 'tight': 'bool', 'abstract': 'bool' } } where 'tight' is a flag indicating whether to set @addrlen to the minimal string length, or the maximum sun_path length. And 'abstract' indicates that we automagically add a leading NUL. This would *not* allow for NULs in the middle of path, but I'm not so bothered about that, since I can't see that being widely used. If we really did need that it could be added via a 'base64': 'bool' flag, to indicate that @path is base64 encoded and thus may contain NULs From a CLI POV, this could be mapped to QAPI thus * -chardev unix:somepath @path==somepath @tight==false @abstract==false * -chardev unix:somepath,tight @path==somepath @tight==true @abstract==false * -chardev unix-abstract:somepath @path==somepath @tight==false @abstract==true * -chardev unix-abstract:somepath,tight @path==somepath @tight==true @abstract==true > > Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com> > --- > util/qemu-sockets.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index bcc06d0e01..7ba9c497ab 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -939,6 +939,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) > struct sockaddr_un un; > int sock, rc; > size_t pathlen; > + socklen_t serverlen; > > if (saddr->path == NULL) { > error_setg(errp, "unix connect: no path specified"); > @@ -963,10 +964,17 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) > un.sun_family = AF_UNIX; > memcpy(un.sun_path, saddr->path, pathlen); > > + if (saddr->path[0] == '@') { > + un.sun_path[0] = '\0'; > + serverlen = pathlen + offsetof(struct sockaddr_un, sun_path); > + } else { > + serverlen = sizeof(un); > + } > + > /* connect to peer */ > do { > rc = 0; > - if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) { > + if (connect(sock, (struct sockaddr *) &un, serverlen) < 0) { > rc = -errno; > } > } while (rc == -EINTR); > -- > 2.17.1 > > Regards, Daniel
在 2020/4/23 下午5:00, Daniel P. Berrangé 写道: > Adding Eric & Markus for QAPI modelling questions > > On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote: >> unix_connect_saddr now support abstract address type >> >> By default qemu does not support abstract UNIX domain >> socket address. Add this ability to make qemu handy >> when abstract address is needed. > Was that a specific app you're using with QEMU that needs this ? Thanks for your reply ! I once use qemu to connect a unix domain socket server (with abstract type address written by android java code) >> Abstract address is marked by prefixing the address name with a '@'. > For full support of the abstract namespace we would ned to allow > the "sun_path" to contain an arbitrary mix of NULs and non-NULs > characters, and allow connect() @addrlen to be an arbitrary size. > > This patch only allows a single initial NUL, and reqiures @addrlen > to be the full size of sun_path, padding with trailing NULs. This > limitation is impossible to lift with QEMU's current approach to > UNIX sockets, as it relies on passing around a NULL terminated > string, so there's no way to have embedded NULs. Since there's > no explicit length, we have to chooose between forcing the full > sun_path size as @addrlen, or forcing the string length as the > @addrlen value. > > IIUC, socat makes the latter decision by default, but has a > flag to switch to the former. > > [man socat] > unix-tightsocklen=[0|1] > On socket operations, pass a socket address length that does not > include the whole struct sockaddr_un record but (besides other compo‐ > nents) only the relevant part of the filename or abstract string. > Default is 1. > [/man] > > This actually is supported for both abstract and non-abstract > sockets, though IIUC this doesn't make a semantic difference > for non-abstract sockets. > > The point is we have four possible combinations > > NON-ABSTRACT + FULL SIZE > NON-ABSTRACT + MINIMAL SIZE (default) > ABSTRACT + FULL SIZE > ABSTRACT + MINIMAL SIZE (default) > > With your patch doing the latter, it means QEMU supports > only two combinations > > NON+ABSTRACT + FULL SIZE > ABSTRACT + MINIMAL SIZE > > and also can't use "@somerealpath" for a non-abstract > socket, though admittedly this is unlikely. > > Socat uses a special option to request use of abstract > sockets. eg ABSTRACT:somepath, and automatically adds > the leading NUL, so there's no need for a special "@" > character. This means that UNIX:@somepath still resolves > to a filesystem path and not a abstract socket path. > > Finally, the patch as only added support for connect() > not listen(). I think if QEMU wants to support abstract > sockets we must do both, and also have unit tests added > to tests/test-util-sockets.c Yes , I missed these parts. > > > The question is whether we're ok with this simple > approach in QEMU, or should do a full approach with > more explicit modelling. Agree, more comments is welcome. > > ie should we change QAPI thus: > > { 'struct': 'UnixSocketAddress', > 'data': { > 'path': 'str', > 'tight': 'bool', > 'abstract': 'bool' } } > > where 'tight' is a flag indicating whether to set @addrlen > to the minimal string length, or the maximum sun_path length. > And 'abstract' indicates that we automagically add a leading > NUL. > > This would *not* allow for NULs in the middle of path, > but I'm not so bothered about that, since I can't see that > being widely used. If we really did need that it could be > added via a 'base64': 'bool' flag, to indicate that @path > is base64 encoded and thus may contain NULs > > From a CLI POV, this could be mapped to QAPI thus > > * -chardev unix:somepath > > @path==somepath > @tight==false > @abstract==false > > * -chardev unix:somepath,tight > > @path==somepath > @tight==true > @abstract==false > > * -chardev unix-abstract:somepath > > @path==somepath > @tight==false > @abstract==true > > * -chardev unix-abstract:somepath,tight > > @path==somepath > @tight==true > @abstract==true > > > > Regards, > Daniel
Eric , Markus, any comments ? 在 2020/4/23 下午6:51, xiaoqiang.zhao 写道: > 在 2020/4/23 下午5:00, Daniel P. Berrangé 写道: >> Adding Eric & Markus for QAPI modelling questions >> >> On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote: >>> unix_connect_saddr now support abstract address type >>> >>> By default qemu does not support abstract UNIX domain >>> socket address. Add this ability to make qemu handy >>> when abstract address is needed. >> Was that a specific app you're using with QEMU that needs this ? > > Thanks for your reply ! > > I once use qemu to connect a unix domain socket server (with abstract > type address written by android java code) > >>> Abstract address is marked by prefixing the address name with a '@'. >> For full support of the abstract namespace we would ned to allow >> the "sun_path" to contain an arbitrary mix of NULs and non-NULs >> characters, and allow connect() @addrlen to be an arbitrary size. >> >> This patch only allows a single initial NUL, and reqiures @addrlen >> to be the full size of sun_path, padding with trailing NULs. This >> limitation is impossible to lift with QEMU's current approach to >> UNIX sockets, as it relies on passing around a NULL terminated >> string, so there's no way to have embedded NULs. Since there's >> no explicit length, we have to chooose between forcing the full >> sun_path size as @addrlen, or forcing the string length as the >> @addrlen value. >> >> IIUC, socat makes the latter decision by default, but has a >> flag to switch to the former. >> >> [man socat] >> unix-tightsocklen=[0|1] >> On socket operations, pass a socket address length that >> does not >> include the whole struct sockaddr_un record but (besides other >> compo‐ >> nents) only the relevant part of the filename or abstract >> string. >> Default is 1. >> [/man] >> >> This actually is supported for both abstract and non-abstract >> sockets, though IIUC this doesn't make a semantic difference >> for non-abstract sockets. >> >> The point is we have four possible combinations >> >> NON-ABSTRACT + FULL SIZE >> NON-ABSTRACT + MINIMAL SIZE (default) >> ABSTRACT + FULL SIZE >> ABSTRACT + MINIMAL SIZE (default) >> >> With your patch doing the latter, it means QEMU supports >> only two combinations >> >> NON+ABSTRACT + FULL SIZE >> ABSTRACT + MINIMAL SIZE >> >> and also can't use "@somerealpath" for a non-abstract >> socket, though admittedly this is unlikely. >> >> Socat uses a special option to request use of abstract >> sockets. eg ABSTRACT:somepath, and automatically adds >> the leading NUL, so there's no need for a special "@" >> character. This means that UNIX:@somepath still resolves >> to a filesystem path and not a abstract socket path. >> >> Finally, the patch as only added support for connect() >> not listen(). I think if QEMU wants to support abstract >> sockets we must do both, and also have unit tests added >> to tests/test-util-sockets.c > Yes , I missed these parts. >> >> >> The question is whether we're ok with this simple >> approach in QEMU, or should do a full approach with >> more explicit modelling. > Agree, more comments is welcome. >> >> ie should we change QAPI thus: >> >> { 'struct': 'UnixSocketAddress', >> 'data': { >> 'path': 'str', >> 'tight': 'bool', >> 'abstract': 'bool' } } >> >> where 'tight' is a flag indicating whether to set @addrlen >> to the minimal string length, or the maximum sun_path length. >> And 'abstract' indicates that we automagically add a leading >> NUL. >> >> This would *not* allow for NULs in the middle of path, >> but I'm not so bothered about that, since I can't see that >> being widely used. If we really did need that it could be >> added via a 'base64': 'bool' flag, to indicate that @path >> is base64 encoded and thus may contain NULs >> >> From a CLI POV, this could be mapped to QAPI thus >> >> * -chardev unix:somepath >> >> @path==somepath >> @tight==false >> @abstract==false >> >> * -chardev unix:somepath,tight >> >> @path==somepath >> @tight==true >> @abstract==false >> >> * -chardev unix-abstract:somepath >> >> @path==somepath >> @tight==false >> @abstract==true >> >> * -chardev unix-abstract:somepath,tight >> >> @path==somepath >> @tight==true >> @abstract==true >> >> >> >> Regards, >> Daniel
On 4/23/20 4:00 AM, Daniel P. Berrangé wrote: > Adding Eric & Markus for QAPI modelling questions > > On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote: >> unix_connect_saddr now support abstract address type >> >> By default qemu does not support abstract UNIX domain >> socket address. Add this ability to make qemu handy >> when abstract address is needed. > > Was that a specific app you're using with QEMU that needs this ? > >> Abstract address is marked by prefixing the address name with a '@'. > > For full support of the abstract namespace we would ned to allow > the "sun_path" to contain an arbitrary mix of NULs and non-NULs > characters, and allow connect() @addrlen to be an arbitrary size. > > This patch only allows a single initial NUL, and reqiures @addrlen > to be the full size of sun_path, padding with trailing NULs. This > limitation is impossible to lift with QEMU's current approach to > UNIX sockets, as it relies on passing around a NULL terminated > string, so there's no way to have embedded NULs. Since there's > no explicit length, we have to chooose between forcing the full > sun_path size as @addrlen, or forcing the string length as the > @addrlen value. > > IIUC, socat makes the latter decision by default, but has a > flag to switch to the former. > > [man socat] > unix-tightsocklen=[0|1] > On socket operations, pass a socket address length that does not > include the whole struct sockaddr_un record but (besides other compo‐ > nents) only the relevant part of the filename or abstract string. > Default is 1. > [/man] > > This actually is supported for both abstract and non-abstract > sockets, though IIUC this doesn't make a semantic difference > for non-abstract sockets. Yes, that matches my experience as well. '^@a' length 2 is a different socket than '^@a^@' length three, but 'a' length 1 and 'a^@' length 2 are the same socket because only non-abstract sockets end at the earlier of the first NUL or the length. > > The point is we have four possible combinations > > NON-ABSTRACT + FULL SIZE > NON-ABSTRACT + MINIMAL SIZE (default) Two combinations, but only one behavior. > ABSTRACT + FULL SIZE > ABSTRACT + MINIMAL SIZE (default) And two more behaviors. > > With your patch doing the latter, it means QEMU supports > only two combinations > > NON+ABSTRACT + FULL SIZE > ABSTRACT + MINIMAL SIZE > > and also can't use "@somerealpath" for a non-abstract > socket, though admittedly this is unlikely. > > Socat uses a special option to request use of abstract > sockets. eg ABSTRACT:somepath, and automatically adds > the leading NUL, so there's no need for a special "@" > character. This means that UNIX:@somepath still resolves > to a filesystem path and not a abstract socket path. Yes, having an additional knob to express abstract sockets seems better than overloading a specific character (although @sockname is a relative name, and relative socket names already come with their own set of problems). > > Finally, the patch as only added support for connect() > not listen(). I think if QEMU wants to support abstract > sockets we must do both, and also have unit tests added > to tests/test-util-sockets.c Indeed. > > > The question is whether we're ok with this simple > approach in QEMU, or should do a full approach with > more explicit modelling. > > ie should we change QAPI thus: > > { 'struct': 'UnixSocketAddress', > 'data': { > 'path': 'str', > 'tight': 'bool', > 'abstract': 'bool' } } You'd want to make 'tight' and 'abstract' optional, with defaults to the existing practice, but otherwise this looks sane to me. > > where 'tight' is a flag indicating whether to set @addrlen > to the minimal string length, or the maximum sun_path length. > And 'abstract' indicates that we automagically add a leading > NUL. > > This would *not* allow for NULs in the middle of path, > but I'm not so bothered about that, since I can't see that > being widely used. If we really did need that it could be > added via a 'base64': 'bool' flag, to indicate that @path > is base64 encoded and thus may contain NULs Agreed that this is less likely to be needed. > >>From a CLI POV, this could be mapped to QAPI thus > > * -chardev unix:somepath > > @path==somepath > @tight==false > @abstract==false > > * -chardev unix:somepath,tight > > @path==somepath > @tight==true > @abstract==false > > * -chardev unix-abstract:somepath > > @path==somepath > @tight==false > @abstract==true > > * -chardev unix-abstract:somepath,tight > > @path==somepath > @tight==true > @abstract==true > Also sounds reasonable to me.
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index bcc06d0e01..7ba9c497ab 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -939,6 +939,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) struct sockaddr_un un; int sock, rc; size_t pathlen; + socklen_t serverlen; if (saddr->path == NULL) { error_setg(errp, "unix connect: no path specified"); @@ -963,10 +964,17 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp) un.sun_family = AF_UNIX; memcpy(un.sun_path, saddr->path, pathlen); + if (saddr->path[0] == '@') { + un.sun_path[0] = '\0'; + serverlen = pathlen + offsetof(struct sockaddr_un, sun_path); + } else { + serverlen = sizeof(un); + } + /* connect to peer */ do { rc = 0; - if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) { + if (connect(sock, (struct sockaddr *) &un, serverlen) < 0) { rc = -errno; } } while (rc == -EINTR);
unix_connect_saddr now support abstract address type By default qemu does not support abstract UNIX domain socket address. Add this ability to make qemu handy when abstract address is needed. Abstract address is marked by prefixing the address name with a '@'. Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com> --- util/qemu-sockets.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)