2 ms·

Fwiw, the analogous logic in musl libc[1] uses log_ident set to the empty string if openlog() was called with NULL ident[2]. It also formats twice to buf[1024]

by loeg 3y ago

Fwiw, the analogous logic in musl libc[1] uses log_ident set to the empty string if openlog() was called with NULL ident[2]. It also formats twice to buf[1024] without checking that the first format's length isn't in excess of the stack buffer but that's likely always safe given how short the stuff printed in the first format is. [1]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n105 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n... [2]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n68 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n...

knorker 3y ago
Interesting. The first snprintf() could return a negative value (at least by the glibc snprintf manpage), and then therefore the second vsnprintf will overwrite... something. Maybe their implementation can't return negative, or only if the fmt string (constant, here) is invalid? I wouldn't count on it, especially of it being true forever. That's pretty sloppy code.
- loeg 3y ago
  I believe the first snprintf only returns a negative value if the format string is broken. Musl has decided to avoid that by not breaking the format string. If that changes, returning -1 is the least of their problems. You can see the implementation here: https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf... and https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.... .
  - bodyfour 3y ago
    Probably the person you're replying to is just confused because before it was standardized some snprintf() implementations returned -1 on overflow. If you were trying to be portable and defensive you'd need to check for either error return. Not really a concern inside musl because those implementations are probably long gone and because it's calling its own snprintf() anyway.
    - knorker 3y ago
      I'm not confused. I know that some snprintf implementations returns/returned -1 on overflow. I assume that musl doesn't, because it's a fine library. > If you were trying to be portable and defensive you'd need to check for either error return. Including defensive against future changes. I'm sure thousands of bugs are being written every day because people don't check return values that "can't happen", because they know the code they call. Then 10-20 years later, someone changes that code they depend on, without violating the contract. I encounter these kinds of bugs all the time. There's a simple way to avoid them: Check the damn return values, even if just with an assert. The extra annoying ones are ones with a comment saying "Can't happen", that then does happen. The person who wrote that could have spent about the same number of characters simply handling the "can't happen". We can't get away from Hyrum's Law, but we sure can try to minimize its impact.
  - knorker 3y ago
    > I believe the first snprintf only returns a negative value if the format string is broken. I encounter bugs all the time that come from the fact that the caller knows the code they call, so they ignore the documented promises. Let's all at least try to not invoke Hyrum's Law, even for code in the same project.
o11c 3y ago
More relevant, it caps `log_ident` to a very short length. If my math is correct, the max length of the first snprintf is 72 bytes (actually less with real-world valid PIDs), and the buffer is 1024. As usual, MUSL is secure at the cost of giving up usefulness. A 32-byte buffer might seem like a lot, but there are dozens of programs with a name longer than 31 bytes on my system. `dbus-update-activation-environment` is the most interesting (most others are gcc/binutils), and I know dbus in general uses syslog, though I haven't traced this particular entry point.
- loeg 3y ago
  Yeah. You have to math out the various fields but I agree it appears to end up a few orders of magnitude smaller than 1024. It's just not particularly explicit. > As usual, MUSL is secure at the cost of giving up usefulness. I would say 'Musl is deliberately simple at the cost of usefulness'. The simplicity lends itself to correctness and security. Btw, since Musl does not implicitly construct log_ident from argv[0], program name is sort of irrelevant unless that program explicitly openlog()s with argv[0]. dbus-update-activation-environment does not appear to openlog or use syslog at all: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools/dbus-update-activation-environment.c https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools... Instead it prints error messages to stderr.
- GoblinSlayer 3y ago
  Truncated to 31 bytes would be dbus-update-activation-environm. Is it ambiguous?

by loeg 3y ago

knorker 3y ago

Interesting. The first snprintf() could return a negative value (at least by the glibc snprintf manpage), and then therefore the second vsnprintf will overwrite... something. Maybe their implementation can't return negative, or only if the fmt string (constant, here) is invalid? I wouldn't count on it, especially of it being true forever. That's pretty sloppy code.

loeg 3y ago
I believe the first snprintf only returns a negative value if the format string is broken. Musl has decided to avoid that by not breaking the format string. If that changes, returning -1 is the least of their problems. You can see the implementation here: https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf... and https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.... .
- bodyfour 3y ago
  Probably the person you're replying to is just confused because before it was standardized some snprintf() implementations returned -1 on overflow. If you were trying to be portable and defensive you'd need to check for either error return. Not really a concern inside musl because those implementations are probably long gone and because it's calling its own snprintf() anyway.
  - knorker 3y ago
    I'm not confused. I know that some snprintf implementations returns/returned -1 on overflow. I assume that musl doesn't, because it's a fine library. > If you were trying to be portable and defensive you'd need to check for either error return. Including defensive against future changes. I'm sure thousands of bugs are being written every day because people don't check return values that "can't happen", because they know the code they call. Then 10-20 years later, someone changes that code they depend on, without violating the contract. I encounter these kinds of bugs all the time. There's a simple way to avoid them: Check the damn return values, even if just with an assert. The extra annoying ones are ones with a comment saying "Can't happen", that then does happen. The person who wrote that could have spent about the same number of characters simply handling the "can't happen". We can't get away from Hyrum's Law, but we sure can try to minimize its impact.
- knorker 3y ago
  > I believe the first snprintf only returns a negative value if the format string is broken. I encounter bugs all the time that come from the fact that the caller knows the code they call, so they ignore the documented promises. Let's all at least try to not invoke Hyrum's Law, even for code in the same project.

o11c 3y ago

More relevant, it caps `log_ident` to a very short length. If my math is correct, the max length of the first snprintf is 72 bytes (actually less with real-world valid PIDs), and the buffer is 1024. As usual, MUSL is secure at the cost of giving up usefulness. A 32-byte buffer might seem like a lot, but there are dozens of programs with a name longer than 31 bytes on my system. `dbus-update-activation-environment` is the most interesting (most others are gcc/binutils), and I know dbus in general uses syslog, though I haven't traced this particular entry point.

loeg 3y ago
Yeah. You have to math out the various fields but I agree it appears to end up a few orders of magnitude smaller than 1024. It's just not particularly explicit. > As usual, MUSL is secure at the cost of giving up usefulness. I would say 'Musl is deliberately simple at the cost of usefulness'. The simplicity lends itself to correctness and security. Btw, since Musl does not implicitly construct log_ident from argv[0], program name is sort of irrelevant unless that program explicitly openlog()s with argv[0]. dbus-update-activation-environment does not appear to openlog or use syslog at all: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools/dbus-update-activation-environment.c https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools... Instead it prints error messages to stderr.
GoblinSlayer 3y ago
Truncated to 31 bytes would be dbus-update-activation-environm. Is it ambiguous?