You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/03/02 23:26:07 UTC
svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Author: rhuijben
Date: Mon Mar 2 22:26:07 2015
New Revision: 1663450
URL: http://svn.apache.org/r1663450
Log:
Following up on r1658194, fix removing tokens from the tokens cache.
Just passing the token to svn_hash_sets() didn't work any more.
* subversion/libsvn_ra_svn/editorp.c
(ra_svn_handle_close_dir,
ra_svn_handle_close_file): Properly handle svn_string_t tokens.
Modified:
subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/editorp.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/editorp.c?rev=1663450&r1=1663449&r2=1663450&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/editorp.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/editorp.c Mon Mar 2 22:26:07 2015
@@ -633,7 +633,7 @@ static svn_error_t *ra_svn_handle_close_
/* Close the directory and destroy the baton. */
SVN_CMD_ERR(ds->editor->close_directory(entry->baton, pool));
- svn_hash_sets(ds->tokens, token, NULL);
+ apr_hash_set(ds->tokens, token->data, token->len, NULL);
svn_pool_destroy(entry->pool);
return SVN_NO_ERROR;
}
@@ -812,7 +812,7 @@ static svn_error_t *ra_svn_handle_close_
/* Close the file and destroy the baton. */
SVN_CMD_ERR(ds->editor->close_file(entry->baton, text_checksum, pool));
- svn_hash_sets(ds->tokens, token, NULL);
+ apr_hash_set(ds->tokens, token->data, token->len, NULL);
if (--ds->file_refs == 0)
svn_pool_clear(ds->file_pool);
return SVN_NO_ERROR;
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 3 March 2015 at 14:17, Julian Foad <ju...@btopenworld.com> wrote:
>>>> URL: http://svn.apache.org/r1663450
>
>>>> Log:
>>>> Following up on r1658194, fix removing tokens from the tokens cache.
>>>> Just passing the token to svn_hash_sets() didn't work any more.
>
> Thinking about how to prevent a repeat of the same kind of error, defining svn_hash_sets and svn_hash_gets as functions with prototypes would result in at least a compiler warning (for typical configurations).
>
> One way of doing this is:
>
> Index: subversion/include/svn_hash.h
> ===================================================================
> --- subversion/include/svn_hash.h (revision 1663350)
> +++ subversion/include/svn_hash.h (working copy)
> @@ -246,2 +246,5 @@
> -#define svn_hash_gets(ht, key) \
> - apr_hash_get(ht, key, APR_HASH_KEY_STRING)
> +static APR_INLINE void *
> +svn_hash_gets(apr_hash_t *ht, const char *key)
> +{
> + return apr_hash_get(ht, key, APR_HASH_KEY_STRING);
> +}
> @@ -253,2 +256,5 @@
> -#define svn_hash_sets(ht, key, val) \
> - apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
> +static APR_INLINE void
> +svn_hash_sets(apr_hash_t *ht, const char *key, const void *val)
> +{
> + apr_hash_set(ht, key, APR_HASH_KEY_STRING, val);
> +}
>
> That particular way would likely trigger 'defined but not used' warnings on some
> source files, with compilers where APR_INLINE is not effective.
>
> Thoughts?
>
I think proposed patch could prevent this particular mistakes, but
this macro is in public header and I'm not sure that this change
doesn't break something.
Anyway I think it's better way to prevent such bugs is to stop
micro-optimizing Subversion. At least think twice before such commits.
I'd like to note that we've just fixed very similar bug with replacing
char * to svn_string_t month ago [1],[2]. And now we got similar
mistake.
[1] http://svn.apache.org/r1483570
[2] http://svn.haxx.se/dev/archive-2015-02/0107.shtml
--
Ivan Zhakov
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Branko Čibej <br...@wandisco.com>.
On 03.03.2015 19:10, Julian Foad wrote:
> Stefan Fuhrmann wrote:
>> Julian Foad wrote:
>>> #define svn_hash_sets(ht, key, val) \
>>> do { \
>>> const char *svn_hash__key = (key); \
>>> apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
>>> } while (0)
>> But it would have caught the problem introduced in r1658194.
>>
>> So, that would be a 50% solution with no obvious drawbacks.
> [...]
>> I can't think of a way to sneak in a type check into the getter
>> without evaluating the key twice.
>>
>> In maintainer mode, though, we could replace the macro(s)
>> with a "proper" function. That would not produce overhead
>> for production code nor would it interfere with API guarantees.
> Yes.
>
> I'll change the 'set' macro as above, and I'll introduce a private 'get' function and make the (public) 'get' macro call the function in debug mode only. (That's a poor man's version of declaring a function as 'inline'.)
>
> Committed: http://svn.apache.org/r1663760
I'd have expected some symmetry in the get/set pair, not function for
one and magic macro for the other; but ... Emerson!
-- Brane
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 3, 2015 at 7:10 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
> > Julian Foad wrote:
> >> #define svn_hash_sets(ht, key, val) \
> >> do { \
> >> const char *svn_hash__key = (key); \
> >> apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
> >> } while (0)
> >
> > But it would have caught the problem introduced in r1658194.
> >
> > So, that would be a 50% solution with no obvious drawbacks.
> [...]
> >
> > I can't think of a way to sneak in a type check into the getter
> > without evaluating the key twice.
> >
> > In maintainer mode, though, we could replace the macro(s)
> > with a "proper" function. That would not produce overhead
> > for production code nor would it interfere with API guarantees.
>
> Yes.
>
> I'll change the 'set' macro as above, and I'll introduce a private 'get'
> function and make the (public) 'get' macro call the function in debug mode
> only. (That's a poor man's version of declaring a function as 'inline'.)
>
> Committed: http://svn.apache.org/r1663760
>
> Thank you!
-- Stefan^2.
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> Julian Foad wrote:
>> #define svn_hash_sets(ht, key, val) \
>> do { \
>> const char *svn_hash__key = (key); \
>> apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
>> } while (0)
>
> But it would have caught the problem introduced in r1658194.
>
> So, that would be a 50% solution with no obvious drawbacks.
[...]
>
> I can't think of a way to sneak in a type check into the getter
> without evaluating the key twice.
>
> In maintainer mode, though, we could replace the macro(s)
> with a "proper" function. That would not produce overhead
> for production code nor would it interfere with API guarantees.
Yes.
I'll change the 'set' macro as above, and I'll introduce a private 'get' function and make the (public) 'get' macro call the function in debug mode only. (That's a poor man's version of declaring a function as 'inline'.)
Committed: http://svn.apache.org/r1663760
- Julian
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 3, 2015 at 5:19 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> [Switching back to plain text]
>
> Stefan Fuhrmann wrote:
> > Julian Foad wrote:
> >> Thinking about how to prevent a repeat of the same kind of error,
> >> defining svn_hash_sets and svn_hash_gets as functions with prototypes
> >> would result in at least a compiler warning (for typical
> configurations).
> >
> > From r1484181 to r1509166, we already had various more sophisticated
> > definitions for those macros. Some of them would have caught the type
> > mismatch. I think it would be safe to change them into something like
> this:
> >
> > #define svn_hash_sets(ht, key, val) \
> > do { \
> > const char *svn_key__temp = (key); \
> > apr_hash_set(ht, svn_key__temp, strlen(svn_key__temp), val); \
> > } while (0)
>
> That would be safe, and works for *set*. We chose not to use strlen, so
> this would be
>
> #define svn_hash_sets(ht, key, val) \
> do { \
> const char *svn_hash__key = (key); \
> apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
> } while (0)
>
But it would have caught the problem introduced in r1658194.
So, that would be a 50% solution with no obvious drawbacks.
> That style of definition doesn't work for *get*.
>
I can't think of a way to sneak in a type check into the getter
without evaluating the key twice.
In maintainer mode, though, we could replace the macro(s)
with a "proper" function. That would not produce overhead
for production code nor would it interfere with API guarantees.
-- Stefan^2.
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Julian Foad <ju...@btopenworld.com>.
[Switching back to plain text]
Stefan Fuhrmann wrote:
> Julian Foad wrote:
>> Thinking about how to prevent a repeat of the same kind of error,
>> defining svn_hash_sets and svn_hash_gets as functions with prototypes
>> would result in at least a compiler warning (for typical configurations).
>
> From r1484181 to r1509166, we already had various more sophisticated
> definitions for those macros. Some of them would have caught the type
> mismatch. I think it would be safe to change them into something like this:
>
> #define svn_hash_sets(ht, key, val) \
> do { \
> const char *svn_key__temp = (key); \
> apr_hash_set(ht, svn_key__temp, strlen(svn_key__temp), val); \
> } while (0)
That would be safe, and works for *set*. We chose not to use strlen, so this would be
#define svn_hash_sets(ht, key, val) \
do { \
const char *svn_hash__key = (key); \
apr_hash_set(ht, svn_hash__key, APR_HASH_KEY_STRING, val); \
} while (0)
That style of definition doesn't work for *get*.
- Julian
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 3, 2015 at 12:17 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> >>> URL: http://svn.apache.org/r1663450
>
> >>> Log:
> >>> Following up on r1658194, fix removing tokens from the tokens cache.
> >>> Just passing the token to svn_hash_sets() didn't work any more.
>
My bad.
Talked about the leak on IRC last night. Bert then bet me on finding
the source and fixing it.
> Thinking about how to prevent a repeat of the same kind of error, defining
> svn_hash_sets and svn_hash_gets as functions with prototypes would result
> in at least a compiler warning (for typical configurations).
>
>From r1484181 to r1509166, we already had various more sophisticated
definitions for those macros. Some of them would have caught the type
mismatch. I think it would be safe to change them into something like this:
#define svn_hash_sets(ht, key, val) \
do { \
const char *svn_key__temp = (key); \
apr_hash_set(ht, svn_key__temp, strlen(svn_key__temp), val); \
} while (0)
-- Stefan^2.
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Julian Foad <ju...@btopenworld.com>.
>>> URL: http://svn.apache.org/r1663450
>>> Log:
>>> Following up on r1658194, fix removing tokens from the tokens cache.
>>> Just passing the token to svn_hash_sets() didn't work any more.
Thinking about how to prevent a repeat of the same kind of error, defining svn_hash_sets and svn_hash_gets as functions with prototypes would result in at least a compiler warning (for typical configurations).
One way of doing this is:
Index: subversion/include/svn_hash.h
===================================================================
--- subversion/include/svn_hash.h (revision 1663350)
+++ subversion/include/svn_hash.h (working copy)
@@ -246,2 +246,5 @@
-#define svn_hash_gets(ht, key) \
- apr_hash_get(ht, key, APR_HASH_KEY_STRING)
+static APR_INLINE void *
+svn_hash_gets(apr_hash_t *ht, const char *key)
+{
+ return apr_hash_get(ht, key, APR_HASH_KEY_STRING);
+}
@@ -253,2 +256,5 @@
-#define svn_hash_sets(ht, key, val) \
- apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
+static APR_INLINE void
+svn_hash_sets(apr_hash_t *ht, const char *key, const void *val)
+{
+ apr_hash_set(ht, key, APR_HASH_KEY_STRING, val);
+}
That particular way would likely trigger 'defined but not used' warnings on some source files, with compilers where APR_INLINE is not effective.
Thoughts?
- Julian
RE: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: dinsdag 3 maart 2015 11:03
> To: dev@subversion.apache.org; Bert Huijben
> Subject: Re: svn commit: r1663450 -
> /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
>
> On 3 March 2015 at 01:26, <rh...@apache.org> wrote:
> > Author: rhuijben
> > Date: Mon Mar 2 22:26:07 2015
> > New Revision: 1663450
> >
> > URL: http://svn.apache.org/r1663450
> > Log:
> > Following up on r1658194, fix removing tokens from the tokens cache.
> > Just passing the token to svn_hash_sets() didn't work any more.
> >
> > * subversion/libsvn_ra_svn/editorp.c
> > (ra_svn_handle_close_dir,
> > ra_svn_handle_close_file): Properly handle svn_string_t tokens.
> >
> Good catch, Bert!
>
> Do I understand correctly that last minute micro-optimization change
> made in r1658194 [1] broke the 1.9.x branch (unbounded memory usage
> and potential access to invalid memory)?
Good catch, Ivan! ;-)
Last night I somehow missed that this was already on 1.9.x. (I was thinking it was one of the patches of early yesterday)
The memory usage is not really unbound, but it does make a hash table point to already freed memory.
Bert
>
> [1] http://svn.apache.org/r1658194
>
> --
> Ivan Zhakov
Re: svn commit: r1663450 - /subversion/trunk/subversion/libsvn_ra_svn/editorp.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 3 March 2015 at 01:26, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Mon Mar 2 22:26:07 2015
> New Revision: 1663450
>
> URL: http://svn.apache.org/r1663450
> Log:
> Following up on r1658194, fix removing tokens from the tokens cache.
> Just passing the token to svn_hash_sets() didn't work any more.
>
> * subversion/libsvn_ra_svn/editorp.c
> (ra_svn_handle_close_dir,
> ra_svn_handle_close_file): Properly handle svn_string_t tokens.
>
Good catch, Bert!
Do I understand correctly that last minute micro-optimization change
made in r1658194 [1] broke the 1.9.x branch (unbounded memory usage
and potential access to invalid memory)?
[1] http://svn.apache.org/r1658194
--
Ivan Zhakov