On 09.02.2015 15:56, Bert Huijben wrote: > >> -----Original Message----- >> From: Evgeny Kotkov [mailto:evgeny.kot...@visualsvn.com] >> Sent: maandag 9 februari 2015 15:06 >> To: Subversion Development >> Subject: Re: svn commit: r1483570 - >> /subversion/trunk/subversion/libsvn_repos/log.c >> >> Stefan Fuhrmann <stef...@apache.org> writes: >> >>> Within libsvn_repos get_log functionality, pass the list of wanted revprops >>> around as an array of svn_string_t* instead of const char*. The added >>> length >>> info allows for more effient functions to be used. Do that. >> [...] >> >>> - char *name = APR_ARRAY_IDX(revprops, i, char *); >>> - svn_string_t *value = svn_hash_gets(r_props, name); >>> - if (censor_revprops >>> - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0 >>> - || strcmp(name, SVN_PROP_REVISION_DATE) == 0)) >>> - /* ... but we can only return author/date. */ >>> - continue; >> [...] >> >>> + const svn_string_t *name >>> + = APR_ARRAY_IDX(revprops, i, const svn_string_t *); >>> + svn_string_t *value >>> + = apr_hash_get(r_props, name->data, name->len); >>> + if (censor_revprops >>> + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR, >>> + name->len) == 0 >>> + || strncmp(name->data, SVN_PROP_REVISION_DATE, >>> + name->len) == 0)) >>> + /* ... but we can only return author/date. */ >>> + continue; >> 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. >> >> I don't know what are the performance implications of using strncmp() in >> favor >> of strcmp(), but the new check will not censor properties like 's', 'sv', ... >> 'svn:a', 'svn:d' and others. This means that we might incorrectly leak these >> revision properties for partially visible revisions. Subversion 1.8.x only >> outputs svn:date / svn:author when perfoming log requests for partially >> visible >> revisions, and *all* other revision properties are censored out, but with >> this >> changeset this is no longer true. >> >> I committed a failing test in r1658406. As for fixing this issue, I think >> that >> we should entirely revert this changeset. > +1
+1; I'm tired of micro-optimizations without proper proof of performance comparisons that also break code. -- Brane
