On 11 June 2015 at 14:02, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> ivan_at_apache.org wrote on Sun, Jun 07, 2015 at 17:07:30 -0000:
>> Author: ivan
>> Date: Sun Jun 7 17:07:30 2015
>> New Revision: 1684047
>>
>> URL: http://svn.apache.org/r1684047
>> Log:
>> Avoid callers of svn__compress() and svn__decompress() construct fake
>> svn_stringbuf_t instance.
>>
[...]
Hi Daniel,
Thanks for review!
>> svn_stringbuf_t *compressed = svn_stringbuf_create_empty(pool);
>> - svn_stringbuf_t *original = svn_stringbuf_create_empty(pool);
>> - original->data = (char *)window->new_data->data; /* won't be modified */
>> - original->len = window->new_data->len;
>> - original->blocksize = window->new_data->len + 1;
>>
>> - SVN_ERR(svn__compress(original, compressed, eb->compression_level));
>> + SVN_ERR(svn__compress(window->new_data->data, window->new_data->len,
>> + compressed, eb->compression_level));
>
> This change introduces the possibility of bugs of the form:
>
> svn_error_t *foo(svn_stringbuf_t *bar, svn_string_t *baz) {
> svn_compress(bar->data, baz->len, ...); /* should be bar->len */
> }
>
> i.e., "bar" and "baz" instead of "bar" and "bar".
>
> I suppose having a svn_string_t formal parameter is an alternative.
>
> It's just a minor issue, of course.
>
I was thinking about adding svn__compress_stringbuf() wrapper around
svn__compress(), but then decided that it doesn't worth efforts given
that svn__compress() used with stringbuf in just few places.
Using svn_string_t is not useful also, because data referenced by
svn_string_t->data must be NUL terminated, which is inconvenient for
callers in some cases. Like stream parser. The old code was passing
incorrect stringbuf_t:
[[[
-static svn_error_t *
-zlib_decode(const unsigned char *in, apr_size_t inLen, svn_stringbuf_t *out,
- apr_size_t limit)
-{
- /* construct a fake string buffer as parameter to svn__decompress.
- This is fine as that function never writes to it. */
- svn_stringbuf_t compressed;
- compressed.pool = NULL;
- compressed.data = (char *)in;
- compressed.len = inLen;
- compressed.blocksize = inLen + 1;
-
- return svn__decompress(&compressed, out, limit);
-}
]]]
What could be useful svn__decompress_stringbuf() with semantic to
decompress buffer in-place. This often happens when FSFS/FSX
decompress revprops that are not compresed by default.
--
Ivan Zhakov
Received on 2015-06-11 13:17:00 CEST