Re: [PATCH] Refactor recv_sideband()

[Lukas Fleischer <lfleischer@lfos.de>]

On Tue, 14 Jun 2016 at 20:11:12, Junio C Hamano wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> > It is not buffered as it writes to stderr. And some C libs do separate 
> > calls to write() for every string format specifier. So "%s%s%c" may end 
> > up calling write() 3 times depending on the implementation.  The example 
> > I gave in commit ed1902ef5c is real and I even observed it with strace 
> > back then.
> 
> I think you meant 9ac13ec9 (atomic write for sideband remote
> messages, 2006-10-11).
> 
> IIRC, back then we did't use to make as much use of strbuf API as we
> do today; if we were doing that commit today, we would be doing
> strbuf, I would suspect.

Thanks for looking up the right commit. I think I might see what is
going on; however, the situation is a bit different to the situation we
had at the time of writing 9ac13ec9 (atomic write for sideband remote
messages, 2006-10-11). Before that, we called write() manually a couple
of times. Now, we use fprintf() which makes stronger guarantees about
thread safety. However, if I understand correctly, there is still one
issue: It seems like in some places, we also directly write() to file
descriptor 2. So the following might happen: An fprintf() implementation
that calls write() once per format specifier is used. The fprintf()
function is called once from one thread, then interrupted between two
write() syscalls. In the meantime, output is written directly to file
descriptor 2 using write().

One possible solution is using strbuf and constructing the message as we
did before. However, that still relies on fprintf() only calling write()
once per format specifier. While that is probably true for all existing
implementations, I don't think it is guaranteed by some standard.
Shouldn't we always use the stderr stream when printing error messages
instead, especially when we care about thread safety?

Regards,
Lukas