[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1684047 - in /subversion/trunk/subversion: include/private/svn_subr_private.h libsvn_delta/svndiff.c libsvn_fs_fs/revprops.c libsvn_fs_x/revprops.c libsvn_subr/compress.c libsvn_subr/packed_data.c

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Thu, 11 Jun 2015 14:16:07 +0300

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

This is an archived mail posted to the Subversion Dev mailing list.