| Message ID | 20170626101159.19676-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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>
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
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.
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) {
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(-)