If the notification message length is 0, ignore the message. #4237
Conversation
| if(n <= 0) { | ||
| log_unit_debug(u, "Got a zero-length notification message. Ignoring"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This function is only called from manager_dispatch_notify_fd(), which you fix below. So an assert is appropriate here, we don't want to check the same condition twice.
There was a problem hiding this comment.
@keszybz out of curiosity, How can you assert that no other function will call this method in a future implementation?
There was a problem hiding this comment.
If somebody adds another call site, they have to look at the function and see what the prerequisites for the arguments are. assert serves as the documentation for that.
There was a problem hiding this comment.
Sorry, but the attitude "the caller must ensure they won't trip an assert() in the callee" is exactly what led to the state where any unprivileged user can take down pid1, precisely because manager_dispatch_notify_fd() should have but did not do that. So unless you've some other means of programatically verifying that it can't/won't happen again, you're leaving a time bomb waiting for the next refactoring that occurs.
| log_warning("Got zero-length notification message. Ignoring."); | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
The (n == 0) check should be after the (n < 0). We check for error conditions first, and then bad input second.
Also, please don't add an empty line here. Since those are all checks for the same operation, they should be adjacent.
Finally, we don't want to warn here, for a repeateable operation performed by some broken service. A debug level log would be OK.
Fixes #4234 Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
|
@keszybz addressed your suggestions. |
keszybz
added
pid1
good-to-merge/waiting-for-ci 👍
|
@keszybz the CI errors seems to be unrelated to the change. |
Fixes #4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
keszybz
removed
the
good-to-merge/waiting-for-ci 👍
|
Applied as 531ac2b with an indentation fix. |
|
@keszybz I think we should ask @poettering why zero-size messages were explicitly allowed, see #4234 (comment) |
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org> (cherry picked from commit 531ac2b)
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org> (cherry picked from commit 531ac2b)
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org> (cherry picked from commit 531ac2b)
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org> Cherry-picked from: 531ac2b Resolves: #1380175
…4237) Fixes systemd#4234. Signed-off-by: Jorge Niedbalski <jnr@metaklass.org>
If the notification message length is 0, write a warning message and ignore.
Fixes #4234
Signed-off-by: Jorge Niedbalski jnr@metaklass.org