You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan <lu...@gmx.de> on 2016/02/01 00:34:32 UTC
[PATCH] Fix for access violation in svn_fs__path_valid()
Hi,
as discussed on the dev list (topic: inconsistent null-ptr handling in
utf-path related functions):
http://mail-archives.apache.org/mod_mbox/subversion-dev/201601.mbox/browser
attached is a patch to prevent svn_fs__path_valid() to run in an access
violation when creating an error due to path being invalid (in this case
NULL).
[[
Prevent an access violation upon an invalid call, while generating an error
message.
* libsvn_fs/fs-loader.c:
(svn_fs__path_valid): add null-check for path-parameter when
generating the
error and use "NULL" in the message, in case
it actually is null.
]]
Regards,
Stefan
Re: [PATCH] Fix for access violation in svn_fs__path_valid()
Posted by Stefan <lu...@posteo.de>.
On 2/9/2016 04:22, Daniel Shahaf wrote:
> Stefan wrote on Sun, Feb 07, 2016 at 20:51:08 +0100:
>> On 2/7/2016 01:22, Daniel Shahaf wrote:
>>> Stefan wrote on Mon, Feb 01, 2016 at 00:34:32 +0100:
>>>> +++ fs-loader.c (working copy)
>>>> @@ -461,7 +461,8 @@
>>>> if (! svn_utf__cstring_is_valid(path))
>>>> {
>>>> return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
>>>> - _("Path '%s' is not in UTF-8"), path);
>>>> + _("Path '%s' is not in UTF-8"),
>>>> + path ? path : "NULL");
>>> Is this actually a problem? svn_error_createf() uses apr_pvsprintf()
>>> which (by code inspection) accepts NULL here.
>> TBH I didn't look further... Just assumed that apr_pvsprintf() would just be
>> a redefine. If it's a specific implementation which checks for NULL here and
>> handles it, then pls disregard my patch.
> https://svn.apache.org/viewvc/apr/apr/tags/1.3.0/memory/unix/apr_pools.c?revision=661875&view=markup#l1100
> https://svn.apache.org/viewvc/apr/apr/tags/1.3.0/strings/apr_snprintf.c?revision=661875&view=markup#l953
>
> I didn't check later versions. (In theory, apr-2.0 might have
> changed this behaviour. I have no reason to think it did.)
Right, with these references found the code in the 1.5.2 source and it's
still the same there (with regards to checking for null-ptrs with %s).
Thanks for digging out the references for me
>
>> P.S. I tried to check out the function definition myself, but could only
>> spot the declaration in the apr source. Couldn't trace down where the
>> function is defined...
> I found it with ctags(1) with -IAPR_DECLARE:
>
> % ctags --extra=+qf --c-types=+p --python-types=-i -IAPR_DECLARE -R .
> % grep apr_pvsprintf tags
> apr_pvsprintf include/apr_strings.h /^APR_DECLARE(char *) apr_pvsprintf(apr_pool_t *p, const char *fmt, va_list ap);$/;" p
> apr_pvsprintf memory/unix/apr_pools.c /^APR_DECLARE(char *) apr_pvsprintf(apr_pool_t *pool, const char *fmt, va_list ap)$/;" f
> % vim -t apr_pvsprintf
>
Was always there in front of me directly. Should have just looked at the
code. At first spot to me it looked as if it wouldn't apply here, since
the definitions were in memory/unix/xxx and I assumed that's just code
which is unix specific... And I got confused by APR_DECLARE() assuming
that these were just forward declarations of these functions...
Now me knows better. ;-)
Regards,
Stefan
Re: [PATCH] Fix for access violation in svn_fs__path_valid()
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan wrote on Sun, Feb 07, 2016 at 20:51:08 +0100:
> On 2/7/2016 01:22, Daniel Shahaf wrote:
> >Stefan wrote on Mon, Feb 01, 2016 at 00:34:32 +0100:
> >>+++ fs-loader.c (working copy)
> >>@@ -461,7 +461,8 @@
> >> if (! svn_utf__cstring_is_valid(path))
> >> {
> >> return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> >>- _("Path '%s' is not in UTF-8"), path);
> >>+ _("Path '%s' is not in UTF-8"),
> >>+ path ? path : "NULL");
> >Is this actually a problem? svn_error_createf() uses apr_pvsprintf()
> >which (by code inspection) accepts NULL here.
> TBH I didn't look further... Just assumed that apr_pvsprintf() would just be
> a redefine. If it's a specific implementation which checks for NULL here and
> handles it, then pls disregard my patch.
https://svn.apache.org/viewvc/apr/apr/tags/1.3.0/memory/unix/apr_pools.c?revision=661875&view=markup#l1100
https://svn.apache.org/viewvc/apr/apr/tags/1.3.0/strings/apr_snprintf.c?revision=661875&view=markup#l953
I didn't check later versions. (In theory, apr-2.0 might have
changed this behaviour. I have no reason to think it did.)
> P.S. I tried to check out the function definition myself, but could only
> spot the declaration in the apr source. Couldn't trace down where the
> function is defined...
I found it with ctags(1) with -IAPR_DECLARE:
% ctags --extra=+qf --c-types=+p --python-types=-i -IAPR_DECLARE -R .
% grep apr_pvsprintf tags
apr_pvsprintf include/apr_strings.h /^APR_DECLARE(char *) apr_pvsprintf(apr_pool_t *p, const char *fmt, va_list ap);$/;" p
apr_pvsprintf memory/unix/apr_pools.c /^APR_DECLARE(char *) apr_pvsprintf(apr_pool_t *pool, const char *fmt, va_list ap)$/;" f
% vim -t apr_pvsprintf
Cheers,
Daniel
Re: [PATCH] Fix for access violation in svn_fs__path_valid()
Posted by Stefan <lu...@posteo.de>.
On 2/7/2016 01:22, Daniel Shahaf wrote:
> Stefan wrote on Mon, Feb 01, 2016 at 00:34:32 +0100:
>> +++ fs-loader.c (working copy)
>> @@ -461,7 +461,8 @@
>> if (! svn_utf__cstring_is_valid(path))
>> {
>> return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
>> - _("Path '%s' is not in UTF-8"), path);
>> + _("Path '%s' is not in UTF-8"),
>> + path ? path : "NULL");
> Is this actually a problem? svn_error_createf() uses apr_pvsprintf()
> which (by code inspection) accepts NULL here.
TBH I didn't look further... Just assumed that apr_pvsprintf() would
just be a redefine. If it's a specific implementation which checks for
NULL here and handles it, then pls disregard my patch.
P.S. I tried to check out the function definition myself, but could only
spot the declaration in the apr source. Couldn't trace down where the
function is defined...
Regards,
Stefan
Re: [PATCH] Fix for access violation in svn_fs__path_valid()
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan wrote on Mon, Feb 01, 2016 at 00:34:32 +0100:
> +++ fs-loader.c (working copy)
> @@ -461,7 +461,8 @@
> if (! svn_utf__cstring_is_valid(path))
> {
> return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> - _("Path '%s' is not in UTF-8"), path);
> + _("Path '%s' is not in UTF-8"),
> + path ? path : "NULL");
Is this actually a problem? svn_error_createf() uses apr_pvsprintf()
which (by code inspection) accepts NULL here.