[Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Daniel P. Berrange]

The 'sun_path' field in the sockaddr_un struct is not required
to be NULL termianted, so when reporting an error, we must use
the separate 'path' variable which is guaranteed terminated.

Fixes a bug spotted by coverity that was introduced in

  commit ad9579aaa16d5b385922d49edac2c96c79bcfb62
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Thu May 25 16:53:00 2017 +0100

    sockets: improve error reporting if UNIX socket path is too long

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 51bf279..37d386f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -930,7 +930,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     strncpy(un.sun_path, path, sizeof(un.sun_path));
 
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
-        error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
+        error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
         goto err;
     }
     if (listen(sock, 1) < 0) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Thomas Huth]

On 26.06.2017 12:11, Daniel P. Berrange wrote:
> The 'sun_path' field in the sockaddr_un struct is not required
> to be NULL termianted, so when reporting an error, we must use

s/NULL/NUL/

NULL is a pointer, NUL is the '\0' character.

> the separate 'path' variable which is guaranteed terminated.
> 
> Fixes a bug spotted by coverity that was introduced in
> 
>   commit ad9579aaa16d5b385922d49edac2c96c79bcfb62
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Thu May 25 16:53:00 2017 +0100
> 
>     sockets: improve error reporting if UNIX socket path is too long
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 51bf279..37d386f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -930,7 +930,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>      strncpy(un.sun_path, path, sizeof(un.sun_path));
>  
>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
> -        error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
> +        error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
>          goto err;
>      }
>      if (listen(sock, 1) < 0) {

Apart from the nit in the comment, the patch looks right to me:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On Mon, Jun 26, 2017 at 12:19:40PM +0200, Thomas Huth wrote:
> On 26.06.2017 12:11, Daniel P. Berrange wrote:
> > The 'sun_path' field in the sockaddr_un struct is not required
> > to be NULL termianted, so when reporting an error, we must use
> 
> s/NULL/NUL/
> 
> NULL is a pointer, NUL is the '\0' character.

I wanted to point out the same thing to someone recently, so I chased up
a reference to the NUL character in RFC 20 "ASCII format for Network
Interchange".  After all, no one can argue with an RFC.

What I found shocked me!  There must be a typo in the ASCII RFC:
https://tools.ietf.org/html/rfc20#section-5.2

I closed my browser tab quickly and headed to Wikipedia instead.  If the
primary source didn't support my argument, I could always count on good
old Wikipedia...

But do you know what I found?  Someone had conflated nul and null on the
Wikipedia entry:
https://en.wikipedia.org/wiki/Null_character

Amateurs!  The Wikipedia editors probably didn't have the intellectual
calibre to question the correctness of the RFC text the way I did.

But to cut a long story short, as my search continued the evidence
became overwhelming.  It is acceptable to refer to the nul character as
the null character.

Coming back to the patch in question, although we can't complain about
the "NULL" it's with considerable joy that I'd like to highlight:

s/termianted/terminated/

:)
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Mon, Jun 26, 2017 at 11:11:59AM +0100, Daniel P. Berrange wrote:
> The 'sun_path' field in the sockaddr_un struct is not required
> to be NULL termianted, so when reporting an error, we must use
> the separate 'path' variable which is guaranteed terminated.
> 
> Fixes a bug spotted by coverity that was introduced in
> 
>   commit ad9579aaa16d5b385922d49edac2c96c79bcfb62
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Thu May 25 16:53:00 2017 +0100
> 
>     sockets: improve error reporting if UNIX socket path is too long
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

On 05.07.2017 15:41, Stefan Hajnoczi wrote:
> On Mon, Jun 26, 2017 at 12:19:40PM +0200, Thomas Huth wrote:
>> On 26.06.2017 12:11, Daniel P. Berrange wrote:
>>> The 'sun_path' field in the sockaddr_un struct is not required
>>> to be NULL termianted, so when reporting an error, we must use
>>
>> s/NULL/NUL/
>>
>> NULL is a pointer, NUL is the '\0' character.
> 
> I wanted to point out the same thing to someone recently, so I chased up
> a reference to the NUL character in RFC 20 "ASCII format for Network
> Interchange".  After all, no one can argue with an RFC.
> 
> What I found shocked me!  There must be a typo in the ASCII RFC:
> https://tools.ietf.org/html/rfc20#section-5.2
> 
> I closed my browser tab quickly and headed to Wikipedia instead.  If the
> primary source didn't support my argument, I could always count on good
> old Wikipedia...
> 
> But do you know what I found?  Someone had conflated nul and null on the
> Wikipedia entry:
> https://en.wikipedia.org/wiki/Null_character
> 
> Amateurs!  The Wikipedia editors probably didn't have the intellectual
> calibre to question the correctness of the RFC text the way I did.
> 
> But to cut a long story short, as my search continued the evidence
> became overwhelming.  It is acceptable to refer to the nul character as
> the null character.

Well, I don't see a real problem here - as long as you write "null" with
lowercase letters. "NULL" with uppercase letters is the pointer. "NUL"
with uppercase letters is the character. And "null" with lowercase
letters is just a context-sensitive word :-)

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] sockets: avoid formatting buffer that may not be NULL terminated

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Jun 26, 2017 at 12:19:40PM +0200, Thomas Huth wrote:
>> On 26.06.2017 12:11, Daniel P. Berrange wrote:
>> > The 'sun_path' field in the sockaddr_un struct is not required
>> > to be NULL termianted, so when reporting an error, we must use
>> 
>> s/NULL/NUL/
>> 
>> NULL is a pointer, NUL is the '\0' character.
>
> I wanted to point out the same thing to someone recently, so I chased up
> a reference to the NUL character in RFC 20 "ASCII format for Network
> Interchange".  After all, no one can argue with an RFC.
>
> What I found shocked me!  There must be a typo in the ASCII RFC:
> https://tools.ietf.org/html/rfc20#section-5.2
>
> I closed my browser tab quickly and headed to Wikipedia instead.  If the
> primary source didn't support my argument, I could always count on good
> old Wikipedia...
>
> But do you know what I found?  Someone had conflated nul and null on the
> Wikipedia entry:
> https://en.wikipedia.org/wiki/Null_character
>
> Amateurs!  The Wikipedia editors probably didn't have the intellectual
> calibre to question the correctness of the RFC text the way I did.
>
> But to cut a long story short, as my search continued the evidence
> became overwhelming.  It is acceptable to refer to the nul character as
> the null character.
>
> Coming back to the patch in question, although we can't complain about
> the "NULL" it's with considerable joy that I'd like to highlight:
>
> s/termianted/terminated/
>
> :)
> Stefan

Despite this shocking, shocking precedence, I formally object to the use
of NULL in the context of C for anything other than "one particular null
pointer".  C's confusing enough without overloading identifiers.