Skip to content

Commit 5fd06fa

Browse files
committed
vreportf: Fix interleaving issues, remove 4096 limitation
This also fixes t5516 on Windows VS build. For detailed explanation please refer to code comments in this commit. There was a lot of back-and-forth already in vreportf(): d048a96 (2007-11-09) - 'char msg[256]' is introduced to avoid interleaving 389d176 (2009-03-25) - Buffer size increased to 1024 to avoid truncation 625a860 (2009-11-22) - Buffer size increased to 4096 to avoid truncation f4c3edc (2015-08-11) - Buffer removed to avoid truncation b5a9e43 (2017-01-11) - Reverts f4c3edc to be able to replace control chars before sending to stderr This fix attempts to solve all issues: 1) avoid multiple fprintf() interleaving 2) avoid truncation 3) avoid char interleaving in fprintf() on some platforms 4) avoid buffer block interleaving when output is large 5) avoid out-of-order messages 6) replace control characters in output Other commits worthy of notice: 9ac13ec (2006-10-11) - Another attempt to solve interleaving. This is seemingly related to d048a96. 137a0d0 (2007-11-19) - Addresses out-of-order for display() 34df8ab (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband() to support UTF-8 emulation eac14f8 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation, so it's safe to use xwrite() again 5e5be9e (2016-06-28) - recv_sideband() uses xwrite() again Signed-off-by: Alexandr Miloslavskiy <[email protected]>
1 parent d966095 commit 5fd06fa

File tree

1 file changed

+148
-6
lines changed

1 file changed

+148
-6
lines changed

usage.c

Lines changed: 148 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,159 @@
66
#include "git-compat-util.h"
77
#include "cache.h"
88

9+
static void replace_control_chars(char* str, size_t size, char replacement)
10+
{
11+
size_t i;
12+
13+
for (i = 0; i < size; i++) {
14+
if (iscntrl(str[i]) && str[i] != '\t' && str[i] != '\n')
15+
str[i] = replacement;
16+
}
17+
}
18+
19+
/*
20+
* Atomically report (prefix + vsnprintf(err, params) + '\n') to stderr.
21+
* Always returns desired buffer size.
22+
* Doesn't write to stderr if content didn't fit.
23+
*
24+
* This function composes everything into a single buffer before
25+
* sending to stderr. This is to defeat various non-atomic issues:
26+
* 1) If stderr is fully buffered:
27+
* the ordering of stdout and stderr messages could be wrong,
28+
* because stderr output waits for the buffer to become full.
29+
* 2) If stderr has any type of buffering:
30+
* buffer has fixed size, which could lead to interleaved buffer
31+
* blocks when two threads/processes write at the same time.
32+
* 3) If stderr is not buffered:
33+
* There are two problems, one with atomic fprintf() and another
34+
* for non-atomic fprintf(), and both occur depending on platform
35+
* (see details below). If atomic, this function still writes 3
36+
* parts, which could get interleaved with multiple threads. If
37+
* not atomic, then fprintf() will basically write char-by-char,
38+
* which leads to unreadable char-interleaved writes if two
39+
* processes write to stderr at the same time (threads are OK
40+
* because fprintf() usually locks file in current process). This
41+
* for example happens in t5516 where 'git-upload-pack' detects
42+
* an error, reports it to parent 'git fetch' and both die() at the
43+
* same time.
44+
*
45+
* Behaviors, at the moment of writing:
46+
* a) libc - fprintf()-interleaved
47+
* fprintf() enables temporary stream buffering.
48+
* See: buffered_vfprintf()
49+
* b) VC++ - char-interleaved
50+
* fprintf() enables temporary stream buffering, but only if
51+
* stream was not set to no buffering. This has no effect,
52+
* because stderr is not buffered by default, and git takes
53+
* an extra step to ensure that in swap_osfhnd().
54+
* See: _iob[_IOB_ENTRIES],
55+
* __acrt_stdio_temporary_buffering_guard,
56+
* has_any_buffer()
57+
* c) MinGW - char-interleaved (console), full buffering (file)
58+
* fprintf() obeys stderr buffering. But it uses old MSVCRT.DLL,
59+
* which eventually calls _flsbuf(), which enables buffering unless
60+
* isatty(stderr) or buffering is disabled. Buffering is not disabled
61+
* by default for stderr. Therefore, buffering is enabled for
62+
* file-redirected stderr.
63+
* See: __mingw_vfprintf(),
64+
* __pformat_wcputs(),
65+
* _fputc_nolock(),
66+
* _flsbuf(),
67+
* _iob[_IOB_ENTRIES]
68+
* 4) If stderr is line buffered: MinGW/VC++ will enable full
69+
* buffering instead. See MSDN setvbuf().
70+
*/
71+
static size_t vreportf_buf(char *buf, size_t buf_size, const char *prefix, const char *err, va_list params)
72+
{
73+
int printf_ret = 0;
74+
size_t prefix_size = 0;
75+
size_t total_size = 0;
76+
77+
/*
78+
* NOTE: Can't use strbuf functions here, because it can be called when
79+
* malloc() is no longer possible, and strbuf will recurse die().
80+
*/
81+
82+
/* Prefix */
83+
prefix_size = strlen(prefix);
84+
if (total_size + prefix_size <= buf_size)
85+
memcpy(buf + total_size, prefix, prefix_size);
86+
87+
total_size += prefix_size;
88+
89+
/* Formatted message */
90+
if (total_size <= buf_size)
91+
printf_ret = vsnprintf(buf + total_size, buf_size - total_size, err, params);
92+
else
93+
printf_ret = vsnprintf(NULL, 0, err, params);
94+
95+
if (printf_ret < 0)
96+
BUG("your vsnprintf is broken (returned %d)", printf_ret);
97+
98+
/*
99+
* vsnprintf() returns _desired_ size (without terminating null).
100+
* If vsnprintf() was truncated that will be seen when appending '\n'.
101+
*/
102+
total_size += printf_ret;
103+
104+
/* Trailing \n */
105+
if (total_size + 1 <= buf_size)
106+
buf[total_size] = '\n';
107+
108+
total_size += 1;
109+
110+
/* Send the buffer, if content fits */
111+
if (total_size <= buf_size) {
112+
replace_control_chars(buf, total_size, '?');
113+
fwrite(buf, total_size, 1, stderr);
114+
}
115+
116+
return total_size;
117+
}
118+
9119
void vreportf(const char *prefix, const char *err, va_list params)
10120
{
121+
size_t res = 0;
122+
char *buf = NULL;
123+
size_t buf_size = 0;
124+
125+
/*
126+
* NOTE: This can be called from failed xmalloc(). Any malloc() can
127+
* fail now. Let's try to report with a fixed size stack based buffer.
128+
* Also, most messages should fit, and this path is faster.
129+
*/
11130
char msg[4096];
12-
char *p;
131+
res = vreportf_buf(msg, sizeof(msg), prefix, err, params);
132+
if (res <= sizeof(msg)) {
133+
/* Success */
134+
return;
135+
}
13136

14-
vsnprintf(msg, sizeof(msg), err, params);
15-
for (p = msg; *p; p++) {
16-
if (iscntrl(*p) && *p != '\t' && *p != '\n')
17-
*p = '?';
137+
/*
138+
* Try to allocate a suitable sized malloc(), if possible.
139+
* NOTE: Do not use xmalloc(), because on failure it will call
140+
* die() or warning() and lead to recursion.
141+
*/
142+
buf_size = res;
143+
buf = malloc(buf_size);
144+
if (buf) {
145+
res = vreportf_buf(buf, buf_size, prefix, err, params);
146+
FREE_AND_NULL(buf);
147+
148+
if (res <= buf_size) {
149+
/* Success */
150+
return;
151+
}
18152
}
19-
fprintf(stderr, "%s%s\n", prefix, msg);
153+
154+
/*
155+
* When everything fails, report in parts.
156+
* This can have all problems prevented by vreportf_buf().
157+
*/
158+
fprintf(stderr, "vreportf: not enough memory (tried to allocate %lu bytes)\n", (unsigned long)buf_size);
159+
fputs(prefix, stderr);
160+
vfprintf(stderr, err, params);
161+
fputc('\n', stderr);
20162
}
21163

22164
static NORETURN void usage_builtin(const char *err, va_list params)

0 commit comments

Comments
 (0)