Potentially incorrect normalization code in new_from_ffi

[Manishearth]

Caught during unsafe review

uds/src/addr.rs

Lines 647 to 654 in a596894

	// normalize addr.len to include terminating NUL byte if possible
	// and not be greater than capacity
	if addr.len >= capacity {
	addr.len = capacity;
	} else if addr.addr.sun_path[(addr.len-1-path_offset()) as usize] != 0 {
	addr.len += 1;
	addr.addr.sun_path[(addr.len-1-path_offset()) as usize] = 0;
	}

Some platforms, including FreeBSD, require a null terminator here, which we are sometimes stripping

e.g. FreeBSD:

The sun_path field must be terminated by a NUL character to be used with SUN_LEN(), but the terminating NUL is not part of the address.

We do have some code on OpenBSD that talks about this but it isn't involved here, and it's only OpenBSD, not FreeBSD as well.

uds/src/addr.rs

Lines 252 to 257 in a596894

	/// Is the size of the underlying `sun_path` field,
	/// minus 1 if the OS is known to require a trailing NUL (`'\0'`) byte.
	pub fn max_path_len() -> usize {
	mem::size_of_val(&Self::new_unspecified().addr.sun_path)
	- if cfg!(target_os="openbsd") {1} else {0}
	}

I'd recommend we'd cautiously not strip the NUL except for specific platforms where we know that that's okay.

In general the NUL invariant is also hard to follow in this follow, would be worth documenting it more.

[tormol]

I don't remember the details here.

While a NUL is required for SUN_LEN(), all OSes I've tested except OpenBSD accepts and returns addresses that are just long enough to not leave space for a NUL within sun_len but otherwise fit. This is tested by max_path_addr() in tests/addr.rs.
I don't know whether it's documented, I assume the worst that can happen if an OS doesn't support it is that you get an error when you try to use it. (As long as it works, any code that uses SUN_LEN() on unknown addresses is in my opinion incorrect.)
The as_raw() functions should document that path addresses might not be NUL-terminated though.

This function is used by all functions that return an address, and has some tests that test it directly. But I see that the error cases are not tested, and that it likely accepts paths one too long on OpenBSD.

I agree the code here is confusing, as I at first thought it was removing the last character from addresses!
It's only stripping NUL if it would be outside of sockaddr_un, but otherwise ensures there is a NUL after it if there is space.
It does include the NUL in the length when there's space for it, is that wrong?
from_path() also does that, so it doesn't seem to cause any problem and I'd be wary of changing it.

[Manishearth]

I assume the worst that can happen if an OS doesn't support it is that you get an error when you try to use it

Could also hit a buffer overrun.

I'd be more comfortable with code here that followed documented semantics.

[tormol]

Is it code calling as_raw() or getting passed an address from it that might have buffer overruns, or are you also worried about the libc functions this library calls?

I want this library to be usable with all addresses that other programs might create.
So if it's only the former, I'd rather deprecate and remove the as_raw() methods than reject usable addresses.

[Manishearth]

cc @maurer @durin42

Yes, I'm worried about this producing bad values that get passed down to libc.

[tormol]

The FreeBSD man page you linked to also says

Unix-domain addresses are variable-length file system pathnames of at most 104 characters.
The include file <sys/un.h> defines this address:

struct sockaddr_un {
    u_char  sun_len;
    u_char  sun_family;
    char    sun_path[104];
};

and combined with your quote it sounds like it's only for using SUN_LEN() that a NUL terminator is required, but that it's otherwise not needed.

The NetBSD man page says the same, and the DragonflyBSD man page doesn't mention SUN_LEN() or NUL.
The Illumos man page however says it should be NUL-terminated, so I'll at least redux max length for it and probably make that the default for unknown OSes.

I haven't found a man page for macOS, but it accepts even longer paths.

[Manishearth]

Yes please, it would be good if the defaults were safe and the deviations from the defaults were strongly documented, linking to these man pages.