On Wed, Oct 29, 2014 at 9:44 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 28.10.2014 14:19, stef...@apache.org wrote: > > Author: stefan2 > > Date: Tue Oct 28 13:19:30 2014 > > New Revision: 1634875 > > > > URL: http://svn.apache.org/r1634875 > > Log: > > Speed up packed revprop access by tuning the manifest file parser. > > [...] > > > +/* Return the minimum length of any packed revprop file name in > REVPROPS. */ > > +static apr_size_t > > +get_min_filename_len(packed_revprops_t *revprops) > > +{ > > + char number_buffer[SVN_INT64_BUFFER_SIZE]; > > + > > + /* The revprop filenames have the format <REV>.<COUNT> - with <REV> > being > > + * at least the first rev in the shard and <COUNT> having at least one > > + * digit. Thus, the minimum is 2 + #decimal places in the start rev. > > + */ > > + return svn__i64toa(number_buffer, revprops->manifest_start) + 2; > > +} > > Are you absolutely sure this is correct? According to the comment, you > should be returning > > strlen(svn_i64toa(...)) + 2 > svn_i64toa returns the number of non-NUL chars written into the buffer provided by the caller. There is no memory allocation. An alternative sequence would look like this: svn_i64toa(buf, val); return strlen(buf)+2; > As it is, you end up allocating buffers that are orders of magnitude > larger than necessary. > The value returned by get_min_filename_len is not used to allocate some buffer but to skip a number of chars in the input buffer during parsing. Correctness is checked afterwards by comparing the number of file names parsed with the number of revisions in the shard; they must match. So, using a value that is too large here is certain to make the parser error out. -- Stefan^2.
