Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c

[Ivan Zhakov <iv...@visualsvn.com>]

On 28 August 2016 at 00:02,  <st...@apache.org> wrote:
> Author: stsp
> Date: Sat Aug 27 21:02:27 2016
> New Revision: 1758069
>
> URL: http://svn.apache.org/viewvc?rev=1758069&view=rev
> Log:
> Fix issue #4652 which shows how to trigger an assertion failure in
> svn_dirent_get_absolute() by passing invalid input on the command line.
>
> Report a proper error message instead.
>
> * subversion/libsvn_subr/dirent_uri.c
>   (svn_dirent_get_absolute): If the caller passed a URL, return an error.
>
Hi Stefan,

I'm not sure that it's proper way to fix the reported reported:
svn_dirent_get_absolute() strictly requires @a absolute to be a
*canonicalized dirent*:
[[[
* Convert @a relative canonicalized dirent to an absolute dirent and
* return the results in @a *pabsolute.
]]]

Passing URL to svn_dirent_get_absolute() is an API violation and
SVN_ERR_ASSERT() is a proper way to react. As far I remember all
svn_dirent_*() functions are only expected to work with filesystem
paths, svn_url_*() functions are designed to work with URLs, while
svn_path_*() functions accepts filesystem path and URLs.

User input should be validated where it's received, i.e. in Subversion
command line client. We also may have problem above the stack, when
this URL is passed to another function that doesn't have an assertion
or something like that.  In this particular case the problem is in
svn_cl__merge() which passes URL as the TARGET_WCPATH argument to
svn_client_merge5().

-- 
Ivan Zhakov

Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c

[Ivan Zhakov <iv...@visualsvn.com>]

On 29 August 2016 at 21:05, Stefan Sperling <st...@elego.de> wrote:
> On Mon, Aug 29, 2016 at 02:44:28PM +0200, Stefan Sperling wrote:
>> If you have time to fix it, please do!
>
> I got an opportunity to fix this myself: http://svn.apache.org/r1758269
Great, thanks! I started looking to this issue, but then was
distracted by another task.


-- 
Ivan Zhakov

Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c

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

On Mon, Aug 29, 2016 at 02:44:28PM +0200, Stefan Sperling wrote:
> If you have time to fix it, please do!

I got an opportunity to fix this myself: http://svn.apache.org/r1758269

Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c

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

On Mon, Aug 29, 2016 at 12:36:05PM +0300, Ivan Zhakov wrote:
> On 28 August 2016 at 00:02,  <st...@apache.org> wrote:
> > Author: stsp
> > Date: Sat Aug 27 21:02:27 2016
> > New Revision: 1758069
> >
> > URL: http://svn.apache.org/viewvc?rev=1758069&view=rev
> > Log:
> > Fix issue #4652 which shows how to trigger an assertion failure in
> > svn_dirent_get_absolute() by passing invalid input on the command line.
> >
> > Report a proper error message instead.
> >
> > * subversion/libsvn_subr/dirent_uri.c
> >   (svn_dirent_get_absolute): If the caller passed a URL, return an error.
> >
> Hi Stefan,
> 
> I'm not sure that it's proper way to fix the reported reported:
> svn_dirent_get_absolute() strictly requires @a absolute to be a
> *canonicalized dirent*:
> [[[
> * Convert @a relative canonicalized dirent to an absolute dirent and
> * return the results in @a *pabsolute.
> ]]]
> 
> Passing URL to svn_dirent_get_absolute() is an API violation and
> SVN_ERR_ASSERT() is a proper way to react. As far I remember all
> svn_dirent_*() functions are only expected to work with filesystem
> paths, svn_url_*() functions are designed to work with URLs, while
> svn_path_*() functions accepts filesystem path and URLs.
> 
> User input should be validated where it's received, i.e. in Subversion
> command line client. We also may have problem above the stack, when
> this URL is passed to another function that doesn't have an assertion
> or something like that.  In this particular case the problem is in
> svn_cl__merge() which passes URL as the TARGET_WCPATH argument to
> svn_client_merge5().
> 
> -- 
> Ivan Zhakov

Hi Ivan,

Yes, I agree. I'll try to fix this soon but I'm travelling at the moment.

If you have time to fix it, please do!

Thanks.