[PATCH] Fix undefined behavior when constructing ID for txn_node_cache in fsfs

[Denis Kovalchuk <de...@visualsvn.com>]

Hello.

I think I have found an undefined behavior in the code that constructs ID for
per-transaction DAG node cache.

In make_txn_root() function, the txn variable of type svn_fs_fs__id_part_t *
is passed to apr_pstrcat() function, leading to the undefined behavior.
I can assume that originally it was planned to pass a textual representation
of the txn, instead of passing the txn itself.

Although ID is not used anywhere, except debug-only calls of
svn_cache__get_info() and svn_cache__format_info() functions, the improper call
to apr_pstrcat() may cause a potential crash, etc.

I have attached a patch that fixes it.

Regards,
Denis Kovalchuk

Re: [PATCH] Fix undefined behavior when constructing ID for txn_node_cache in fsfs

[Nathan Hartman <ha...@gmail.com>]

On Thu, Apr 2, 2020 at 11:29 AM Denis Kovalchuk
<de...@visualsvn.com> wrote:
> I think I have found an undefined behavior in the code that constructs ID for
> per-transaction DAG node cache.
>
> In make_txn_root() function, the txn variable of type svn_fs_fs__id_part_t *
> is passed to apr_pstrcat() function, leading to the undefined behavior.
> I can assume that originally it was planned to pass a textual representation
> of the txn, instead of passing the txn itself.
>
> Although ID is not used anywhere, except debug-only calls of
> svn_cache__get_info() and svn_cache__format_info() functions, the improper call
> to apr_pstrcat() may cause a potential crash, etc.

Good catch!

This definitely looks like a bug, that was never caught because
apr_pstrcat() is a variadic function with NO type checking!!

Thanks,
Nathan

Re: [PATCH] Fix undefined behavior when constructing ID for txn_node_cache in fsfs

[Stefan Sperling <st...@elego.de>]

On Thu, Apr 02, 2020 at 06:29:06PM +0300, Denis Kovalchuk wrote:
> Hello.
> 
> I think I have found an undefined behavior in the code that constructs ID for
> per-transaction DAG node cache.
> 
> In make_txn_root() function, the txn variable of type svn_fs_fs__id_part_t *
> is passed to apr_pstrcat() function, leading to the undefined behavior.
> I can assume that originally it was planned to pass a textual representation
> of the txn, instead of passing the txn itself.
> 
> Although ID is not used anywhere, except debug-only calls of
> svn_cache__get_info() and svn_cache__format_info() functions, the improper call
> to apr_pstrcat() may cause a potential crash, etc.
> 
> I have attached a patch that fixes it.
> 
> Regards,
> Denis Kovalchuk


Thank you. Committed in r1876054 (I trimmed one line in your log message
that was longer than 80 coloumns wide).

I will also nominate this for backports.

> Fix undefined behavior when constructing ID for txn_node_cache in fsfs.
> 
> In make_txn_root() function, the txn variable of type svn_fs_fs__id_part_t *
> is passed to apr_pstrcat() function, leading to the undefined behavior.
> I can assume that originally it was planned to pass a textual representation
> of the txn, instead of passing the txn itself.
> 
> Although ID is not used anywhere, except debug-only calls of
> svn_cache__get_info() and svn_cache__format_info() functions, the improper call
> to apr_pstrcat() may cause a potential crash, etc.
> 
> * subversion/libsvn_fs_fs/tree.c
>   (make_txn_root): Pass a textual representation of the txn to apr_pstrcat() function.
> 
> Patch by: Denis Kovalchuk <de...@visualsvn.com>
> 
> Index: subversion/libsvn_fs_fs/tree.c
> ===================================================================
> --- subversion/libsvn_fs_fs/tree.c	(revision 1875922)
> +++ subversion/libsvn_fs_fs/tree.c	(working copy)
> @@ -4644,7 +4644,7 @@ make_txn_root(svn_fs_root_t **root_p,
>                                        svn_fs_fs__dag_deserialize,
>                                        APR_HASH_KEY_STRING,
>                                        32, 20, FALSE,
> -                                      apr_pstrcat(pool, txn, ":TXN",
> +                                      apr_pstrcat(pool, root->txn, ":TXN",
>                                                    SVN_VA_NULL),
>                                        root->pool));
>