On 04/24/2009 10:10 PM, C. Michael Pilato wrote:
> [Please Cc: me in responses -- I think I still have APR commit privs, but
> I'm not active here and not subscribed to the mailing lists.]
>
> In the past couple of weeks, I've seen two different reports of what appears
> to be corruption in the stream of data transmitted by Subversion's
> mod_dav_svn through Apache and back to the Subversion client. What is seen
> client-side is an opening XML tag, a truncated bit of CDATA "inside" the
> tag, and then a missing XML closing tag. The problem seems to occur with
> magically sized chunks of data, so it can be hard to reproduce[1].
>
> Here are the relevant pieces of the call stack:
>
> mod_dav_svn/reports/replay.c's change_file_or_dir_prop() function contains
> the following (which is base64-encoding Subversion file and directory
> properties, and tossing them into an XML REPORT request response):
>
> const svn_string_t *enc_value = svn_base64_encode_string2(value, TRUE,
> pool);
> SVN_ERR(dav_svn__send_xml
> (eb->bb, eb->output,
> "<S:change-%s-prop name=\"%s\">%s</S:change-%s-prop>" DEBUG_CR,
> file_or_dir, qname, enc_value->data, file_or_dir));
>
> dav_svn__send_xml() is a wrapper around apr_brigade_vprintf().
>
> As you know, apr_brigade_vprintf() (in buckets/apr_brigade.c) looks like so:
>
> APU_DECLARE(apr_status_t) apr_brigade_vprintf(apr_bucket_brigade *b,
> apr_brigade_flush flush,
> void *ctx,
> const char *fmt, va_list va)
> {
> /* the cast, in order of appearance */
> struct brigade_vprintf_data_t vd;
> char buf[APR_BUCKET_BUFF_SIZE];
> int written;
>
> vd.vbuff.curpos = buf;
> vd.vbuff.endpos = buf + APR_BUCKET_BUFF_SIZE;
> vd.b = b;
> vd.flusher = &flush;
> vd.ctx = ctx;
> vd.cbuff = buf;
>
> written = apr_vformatter(brigade_flush, &vd.vbuff, fmt, va);
>
> if (written == -1) {
> return -1;
> }
>
> /* tack on null terminator to remaining string */
> *(vd.vbuff.curpos) = '\0';
>
> /* write out what remains in the buffer */
> return apr_brigade_write(b, flush, ctx, buf, vd.vbuff.curpos - buf);
> }
>
> The function apr_vformatter() uses the buffer "buf" to format the string.
> This function in turn uses the macro INS_CHAR to add characters to the
> buffer. INS_CHAR is defined like this:
>
> #define INS_CHAR(c, sp, bep, cc) \
> { \
> if (sp) { \
> if (sp >= bep) { \
> vbuff->curpos = sp; \
> if (flush_func(vbuff)) \
> return -1; \
> sp = vbuff->curpos; \
> bep = vbuff->endpos; \
> } \
> *sp++ = (c); \
> } \
> cc++; \
> }
>
> So, when the macro is executed to add a new character to the buffer and the
> buffer is full, the flush function is called to make room for the new
> character, and then the character is added. Of course, if the buffer has
> room for exactly one more character, it is not flushed, the character is
> added, and the current position of the buffer is at its end (which is
> actually one byte beyond the space allocated for the buffer).
>
> After the call to apr_vformatter(), there will be stuff in the buffer. In
> the special case above, the buffer may be perfectly full (perhaps after
> having been flushed one or more times, but still full now). Then, without
> checking for that condition, this line is executed:
>
> /* tack on null terminator to remaining string */
> *(vd.vbuff.curpos) = '\0';
>
> Uh-oh. Buffer overflow!
>
> Our CollabNet engineer is proposing a simple fix: defining 'buf' inside
> apr_brigade_vprintf() like so:
>
> char buf[APR_BUCKET_BUFF_SIZE + 1]
>
> (Note the "+ 1" to make room for that pesky NULL byte.)
>
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all. Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write? Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data? Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".
Thanks for the detailed analysis. IMHO the best fix is to simply remove
*(vd.vbuff.curpos) = '\0';
apr_brigade_write does not expect a 'string' that means a sequence
of bytes with '\0' marking its end. It expects a buffer of a given length.
The way it is called by apr_brigade_vprintf it does never ever now that the
buffer it gets is followed by '\0' and it does not copy this '\0' to the
brigade.
So just drop it.
Regards
Rüdiger
