setproctitle() unsafe use of vsprintf

[MASHtm]

I had some SIGABRTs recently and since the code in lib/setproctitle.c didn't change up to HEAD I wanted to ask your opinion on the use of vsprintf() in setproctitle() there. It uses a fixed sized destination and has to sprintf cmd.s. IMO (and the coredump suggests it as well) I had these SIGABRTs in vsprintf() because cmd.s was too large, because somebody tried some "HTTP fuzzing" on my imap and pop3 ports.

[elliefm]

Yeah that does look unsafe. Something like this might sort it out:

diff --git a/lib/setproctitle.c b/lib/setproctitle.c
index d86c9283d..cd575b704 100644
--- a/lib/setproctitle.c
+++ b/lib/setproctitle.c
@@ -251,7 +251,7 @@ setproctitle(const char *fmt __attribute__((__unused__)), ...)
 
         /* print the argument string */
         VA_START(fmt);
-        (void) vsprintf(p, fmt, ap);
+        (void) vsnprintf(p, SPT_BUFSIZE, fmt, ap);
         VA_END;
 
         i = strlen(buf);

Not sure how to test this though. I can't remember how the setproctitle implementation hangs together... looks like there's a mess of things to define with -D in CFLAGS to enable and configure it, but it might also just use a libc one automatically if it's there (which wouldn't be affected by the bug in our implementation).

[MASHtm]

Yes, I added -DUSE_SETPROCTITLE to CFLAGS in my spec file. I think a "closing" null byte should also be forced.

[elliefm]

That's not necessary, the snprintf and vsnprintf functions guarantee to always null-terminate the string. If the string and null byte won't both fit in the buffer together, the string is truncated until they do.