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

RE: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c

From: Bert Huijben <bert_at_qqmail.nl>
Date: Fri, 28 Mar 2014 19:47:04 +0100

> -----Original Message-----
> From: philip_at_apache.org [mailto:philip_at_apache.org]
> Sent: vrijdag 28 maart 2014 18:51
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-
> loader.c
>
> Author: philip
> Date: Fri Mar 28 17:51:09 2014
> New Revision: 1582845
>
> URL: http://svn.apache.org/r1582845
> Log:
> Fix a potential error leak.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_lock, svn_fs_unlock): Handle errors in a manner similar to
> the repos functions.
>
> Found by: julianfoad
>
> Modified:
> subversion/trunk/subversion/libsvn_fs/fs-loader.c
>
> Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-
> loader.c?rev=1582845&r1=1582844&r2=1582845&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs/fs-loader.c (original)
> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Fri Mar 28 17:51:09
> 2014
> @@ -1677,21 +1677,30 @@ svn_fs_lock(svn_lock_t **lock, svn_fs_t
> {
> apr_hash_t *targets = apr_hash_make(pool), *results;
> svn_fs_lock_target_t target;
> - svn_fs_lock_result_t *result;
> + svn_error_t *err;
>
> target.token = token;
> target.current_rev = current_rev;
> svn_hash_sets(targets, path, &target);
>
> - SVN_ERR(svn_fs_lock2(&results, fs, targets, comment, is_dav_comment,
> - expiration_date, steal_lock, pool, pool));
> + err = svn_fs_lock2(&results, fs, targets, comment, is_dav_comment,
> + expiration_date, steal_lock, pool, pool);
> +
> + if (apr_hash_count(results))

Is this function explicitly documented to always set the results value on all error paths?

I don't see that in the documentation for svn_fs_lock2?

And it is certainly not our standard pattern for public functions... By default the output arguments are undefined on error.

(It looks like all the implementations handle things this way...)

        Bert

> + {
> + svn_fs_lock_result_t *result
> + = svn__apr_hash_index_val(apr_hash_first(pool, results));
>
> - SVN_ERR_ASSERT(apr_hash_count(results));
> - result = svn__apr_hash_index_val(apr_hash_first(pool, results));
> - if (result->lock)
> - *lock = result->lock;
> + if (result->lock)
> + *lock = result->lock;
> +
> + if (err && result->err)
> + svn_error_compose(err, result->err);
> + else if (!err)
> + err = result->err;
> + }
>
> - return result->err;
> + return err;
> }
>
> svn_error_t *
> @@ -1717,18 +1726,26 @@ svn_fs_unlock(svn_fs_t *fs, const char *
> svn_boolean_t break_lock, apr_pool_t *pool)
> {
> apr_hash_t *targets = apr_hash_make(pool), *results;
> - svn_fs_lock_result_t *result;
> + svn_error_t *err;
>
> if (!token)
> token = "";
> svn_hash_sets(targets, path, token);
>
> - SVN_ERR(svn_fs_unlock2(&results, fs, targets, break_lock, pool, pool));
> + err = svn_fs_unlock2(&results, fs, targets, break_lock, pool, pool);
> +
> + if (apr_hash_count(results))
> + {
> + svn_fs_lock_result_t *result
> + = svn__apr_hash_index_val(apr_hash_first(pool, results));
>
> - SVN_ERR_ASSERT(apr_hash_count(results));
> - result = svn__apr_hash_index_val(apr_hash_first(pool, results));
> + if (err && result->err)
> + svn_error_compose(err, result->err);
> + else if (!err)
> + err = result->err;
> + }
>
> - return result->err;
> + return err;
> }
>
> svn_error_t *
>
Received on 2014-03-28 19:47:57 CET

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