Potentially incorrect normalization code in new_from_ffi

Caught during unsafe review

https://github.com/tormol/uds/blob/a5968947e08cc092b417f30f12bd3d9cf3b4c981/src/addr.rs#L647-L654

Some platforms, including FreeBSD, require a null terminator here, which we are sometimes stripping

e.g. [FreeBSD](https://man.freebsd.org/cgi/man.cgi?query=unix&sektion=4):

> 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.

https://github.com/tormol/uds/blob/a5968947e08cc092b417f30f12bd3d9cf3b4c981/src/addr.rs#L252-L257

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.

	// 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;
	}

	/// 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}
	}

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Potentially incorrect normalization code in new_from_ffi #16

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Potentially incorrect normalization code in new_from_ffi #16

Description

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions