search

lucab / libsystemd-rs

A pure-Rust client library to work with systemd
https://crates.io/crates/libsystemd
Other
106 stars 19 forks source link

NotifyState::Fdname uses String, but FDNAME does not support UTF-8 #116

Closed janderholm closed 1 year ago

janderholm commented 1 year ago

FDNAME=… ... The name may consist of arbitrary ASCII characters except control characters or ":". It may not be longer than 255 characters. If a submitted name does not follow these restrictions, it is ignored.

https://www.freedesktop.org/software/systemd/man/sd_notify.html#FDNAME=%E2%80%A6

So while sending UTF-8 won't break things entirely, the name will be silently ignored which may be confusing and cause hard to find bugs.

lucab commented 1 year ago

Thanks for the report. Good catch, I hadn't notice this restriction before! Did you hit this in the wild?

I think one option is to replace the inner String with some stricter type that we introduce ad-hoc for that single usage. This is probably the most correct solution, but it doesn't look very ergonomic.

Another option would be to keep the enum as it is for the moment, and introduce some internal validation on the fdname. It should immediately help detecting such bugs without being too much invasive, but at the same time it could become a performance hit (I'm thinking watchdog continuous signaling, specifically).

I'm inclined to start doing the latter, but I'm open to suggestions.

swsnr commented 1 year ago

I think I'd just document the restriction. We can't catch every mistake, and there's no readily available type we could use here.

janderholm commented 1 year ago

@lucab no I didn't catch it in the wild. I just read the man sd_notify rather carefully when I couldn't get my example send_fds example working!

Can not "notify" be made to verify strings and return an error if something invalid is being sent? It seems to me that FDNAME would mostly be used with short descriptive names. So if an error is returned, the alternative would have been that it goes undetected which is probably worse.

swsnr commented 1 year ago

Can not "notify" be made to verify strings and return an error if something invalid is being sent? It seems to me that FDNAME would mostly be used with short descriptive names. So if an error is returned, the alternative would have been that it goes undetected which is probably worse.

You mean returning an SdError("Invalid FDNAME") if the name's longer than 255 characters or contains any non-ASCII or control character?

janderholm commented 1 year ago

Yes something like that. Either way there's going to be an error, just if no validation is performed it's a silent error.

swsnr commented 1 year ago

Would you make a pull request?