Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c

On Mon, Feb 9, 2015 at 4:52 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
wrote:

> Stefan Fuhrmann <stefan2_at_apache.org> writes:
>
> >> As it turns out, this particular micro-optimization makes a data leak
> >> possible. This is not a real security issue, as the change happened on
> >> trunk and didn't become part of any released version. Still, I think
> >> that we should fix this prior to making 1.9 public.
> >
> > Good catch, Evgeny! Fixed in r1658439.
>
> I wonder if we really should be adding static const svn_string_t's, and
> using
> "sizeof(define) -1" for unknown performance improvements without
> appropriate
> measurements.

I had two basic options to fix the bug.
(1) Use svn_string_compare that is there for exactly that purpose.
(2) Fall back to low-level C-string ops and hope to get it right this time.

I chose with option (1). We use the "sizeof(define) -1" construct
in at least 32 other places already. I simply went the extra mile
and documented it, which makes it look like something out of
the ordinary.

> By the way, it looks like this changeset [1] did not really
> remove the XFail() marker from the new test, so I expect the buildbots to
> turn red.
>

Yes, I noticed that and fixed it in r1658442.

-- Stefan^2.
Received on 2015-02-11 11:30:27 CET