Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Subject: Re: [PATCH] Fix for access violation in svn_fs__path_valid()
To: Daniel Shahaf <d.s@daniel.shahaf.name>
References: <56AE9A08.6090806@gmx.de> <20160207002206.GD13224@tarsus.local2>
 <56B7A02C.1010705@posteo.de> <20160209032259.GB19531@tarsus.local2>
Cc: dev@subversion.apache.org
From: Stefan <luke1410@posteo.de>
Message-ID: <56BA663A.7040300@posteo.de>
Date: Tue, 9 Feb 2016 23:20:42 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.1
MIME-Version: 1.0
In-Reply-To: <20160209032259.GB19531@tarsus.local2>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

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