Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c
On 29 August 2016 at 21:05, Stefan Sperling 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
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
On Mon, Aug 29, 2016 at 12:36:05PM +0300, Ivan Zhakov wrote: > On 28 August 2016 at 00:02, 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.
Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c
On 28 August 2016 at 00:02, 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