Created attachment 95270 [details] [review]
Fix use of printf on W32

Don't print FROM and TO the same string. None of the printf implementations
that i've tried support this.

Use snprintf instead of printf, to avoid buffer overruns.

> 0005-fix-printf-madness.mingw.patch:
> 
> This looks like an incomplete fix for
> https://bugs.freedesktop.org/show_bug.cgi?id=35311 , which also has
> an incomplete patch that I have reviewed. Please follow up there if
> you are interested in fixing this bug. I would much prefer to use the
> safer DBusString idiom, like I sketched out there: we have this
> string-buffer abstraction for very good reasons, let's use it.
> 
> I note in passing that this implementation appears to have one of the
> same bugs as the one I reviewed on Bugzilla: if the result of (in
> Python-like pseudocode) s + " " + (msg % args) is exactly 1024 bytes
> long, then it calls OutputDebugStringA(buf) while buf contains 1024
> nonzero bytes and no zero-termination. This sort of thing is exactly
> why we avoid direct use of the (v)snprintf() family in
> security-sensitive code :-)
> 
OK, it probably warrants to truncate "printed" to 1023 if it's more than that. And probably force 1024th byte to be zero just in case snprintf() implementation doesn't properly terminate the string.

If you want a more in-depth fix for all printing calls, you're welcome to come up with something, i'll test that if needed. This patch is just to quickly fix a blatantly obvious problem that leads to a crash.

(In reply to comment #2)
> OK, it probably warrants to truncate "printed" to 1023 if it's more than
> that. And probably force 1024th byte to be zero just in case snprintf()
> implementation doesn't properly terminate the string.

Sorry, I may have been wrong about this - http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf implies that snprintf() only prints the first n-1 characters, followed by \0, so it *is* guaranteed to terminate. I think I was mixing it up with strncpy(), which isn't guaranteed to terminate the string.

I'd still be much, much happier with DBusString.

> If you want a more in-depth fix for all printing calls, you're welcome to
> come up with something, i'll test that if needed. This patch is just to
> quickly fix a blatantly obvious problem that leads to a crash.

There is a practical problem here: I'm the most active reviewer, so if I write a patch, nobody is going to review it, and it will probably sit in Bugzilla forever. :-)

(I also don't use Windows.)

Comment on attachment 95270 [details] [review]
Fix use of printf on W32

Review of attachment 95270 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-util-win.c
@@ +316,1 @@
>    OutputDebugStringA(buf);

It's not at all clear to me that this is even an appropriate place for the equivalent of a syslog message to go. Shouldn't it be going to the Windows event log, or something, rather than being considered debug output?

I suspect that nobody actually uses this functionality on Windows, except in its regression test - there's no system bus on Windows, and the system bus is the main place where configuring for <syslog/> is useful.

(In reply to comment #2)
> OK, it probably warrants to truncate "printed" to 1023 if it's more than
> that.

I don't think there's necessarily any need for that - the string being printed is one of three short, hard-coded words, so the only way @printed could be more than 9 is if snprintf() was completely broken, at which point all bets are off.

I'm in two minds about this. Your patch does appear to be technically correct, but I'd much rather have an implementation that is obviously correct and doesn't require thought.

Alternatively, if _dbus_system_log() is never actually useful on Windows (because it outputs to a debugger that nobody will look at), it might be better to make it a no-op.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/99.