malformed unix socket filename

[kaliber]

Attached code creates 'socket_test_filenam\302^K\200\C' instead of 'socket_test_filename'. It works on linux.

gcc socket.c -lnetwork

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>

#define SOCK_PATH "socket_test_filename"

int main(void)
{
    int s, len;
    struct sockaddr_un local;
    char str[100];

    if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
        perror("socket");
        exit(1);
    }

    local.sun_family = AF_UNIX;
    strcpy(local.sun_path, SOCK_PATH);
    unlink(local.sun_path);
    len = strlen(local.sun_path) + sizeof(local.sun_family);
    if (bind(s, (struct sockaddr *)&local, len) == -1) {
        perror("bind");
        exit(1);
    }
    return 0;
}

[axeld]

The code is incorrect anyway, as you make assumptions about the contents of sockaddr_un - on Haiku, it also has a length field that you miss on the length computation, causing the terminating null byte to be cut off.

Please always just use sizeof(sockaddr_un) for the length.

[kaliber]

Oh, my mistake, sorry. The correct length should be:
len = offsetof(struct sockaddr_un, sun_path) + strlen(local.sun_path)

But seems there is a bug in haiku too. Here is more information:
​http://bugs.freedesktop.org/show_bug.cgi?id=19445

[axeld]

Indeed, that looks like a bug, thanks for the note and persistence! :-)
BTW it's called "Haiku", not "HaikuOS".

[michaelvoliveira]

Maybe our sys/un.h implementation differs from Posix one?

Here
​http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/un.h.html

here is the opensolaris implementation
​http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/uts/common/sys/un.h

[bonefish]

I guess I would still consider the sample code invalid. ​bind()'s address_len parameter "Specifies the length of the sockaddr structure pointed to by the address argument." I would interpret that to mean sizeof(sockaddr_un) in this case. The sun_len field doesn't have to be set. It will be ignored anyway (not doing so would break POSIX compliant programs).

That being said it certainly wouldn't harm to consider the address size passed as a limit. Currently the address is copied into a a sockaddr_storage and strnlen(..., sizeof(sockaddr_un)) is used to determine the actual length of the address. Furthermore the value the unix module writes to the sun_len field actually includes the terminating null, which other OSs don't include. And finally the SUN_LEN() macro could be added to <sys/un.h> (several other OSs have it).

To sum it up, while I think the reported problem is invalid I'll leave the ticket open for sake of the related issues/features.

[akira]

I found this issue while I compare various Unix domain socket implementation.
​https://github.com/akr/socket-test

I agree that the sample code is not POSIX compliant because
POSIX defines sun_path field as a pathname and pathname is NUL-terminated by
definition.

However, specifying a length without NUL is a traditional way to
use Unix domain socket address.

For example, a 4.3BSD document describes that the length doesn't contains NUL.

​http://www.tuhs.org/Archive/4BSD/Distributions/4.3BSD/usr.tar.gz
doc/ps1/08.ipc/2.t

       #include <sys/un.h>
        ...
       struct sockaddr_un addr;
        ...
       strcpy(addr.sun_path, "/tmp/foo");
       addr.sun_family = AF_UNIX;
       bind(s, (struct sockaddr *) &addr, strlen(addr.sun_path) +
           sizeof (addr.sun_family));

  Note  that  in determining the size of a UNIX domain address
  null bytes are not counted, which is why strlen is used.

4.4BSD document also has similar description.
(It adds sizeof(addr.sun_len) though)
​http://docs.freebsd.org/44doc/psd/21.ipc/paper.pdf

So Haiku is not friendly for traditional programs.

Also, out-of-bounds read is not a good programming practice.
CWE-125: Out-of-bounds Read
​http://cwe.mitre.org/data/definitions/125.html

What interesting about Haiku is Out-of-bounds Read is happen on
copied socket address.
This means NUL just after specified length cannot prevent the problem.

[jscipione]

NUL terminate unix socket address in UnixEndpoint.cpp

[pdziepak]

The patch I've just attached solves the compatibility problem with applications expecting bind() and connect() to accept non-null-terminated strings. Since it affects only system calls anyone using UNIX sockets in kernel mode (probably an unlikely scenario) would still need to pass a null-terminated string.

The example code in this ticket still truncates the last 'e' in the path since it incorrectly assumes that sockaddr_un contains no other fields than sun_family and sun_path. I don't think there is much we can do about it.

[pdziepak]

Patch commited in hrev46000.

Since the issue with null-termination is now solved and the problems resulting from the presence of sa_len are not really Haiku fault I guess this ticket may be closed now.