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 Küng <to...@gmail.com> on 2008/07/28 14:33:31 UTC
NULL-pointer access when merging with 1.5.1
Hi,
The following conditions will trigger a NULL-pointer access:
* svn library 1.5.1 (I guess 1.5.0 too)
* merge results in a conflict
* user provides his own 'resolved' file in the conflict resolver callback
* the users own 'resolved' file has a path *not* in the same folder than
the conflicted file is located (best use a completely different path)
Since TortoiseSVN always creates the 'resolved' file in the %TEMP%
folder, every attempt to resolve a merge conflict in the callback leads
to a segfault.
The reason for this:
subversion\libsvn_wc\log.c : svn_wc__loggy_copy()
calls the function loggy_path() with those paths. But in loggy_path(),
if the path to the resolved file is not a child of the folder where the
conflict happened, it returns a NULL pointer as the string.
It seems that loggy_path() is a dangerous function: it assumes that
paths are relative to each other. For the 'resolved' file path, this
already leads to a segfault. But I assume that this causes other
problems with TSVN which I haven't been able yet to determine the exact
reason. Remember: UI applications don't have the 'current working
directory' set to what the command line client has it set. In TSVN, I
had to set the CWD to the %TEMP% folder because some svn functions write
temp files to the CWD - and if I don't set the CWD it can be anything,
even the SYSTEM32 folder which is write protected.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: NULL-pointer access when merging with 1.5.1
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Bernhard Merkle wrote:
> Heh. I've been trying for *months* to get access to those results, but to
>> no avail. If Coverity is scanning our source code, it sure doesn't do us
>> much good to not have the results of the scan.
>>
>
> Have you submitted it for inclusion on http://scan.coverity.com/
> or did they do this internally and give you the result ?
>
> As open source project, it is preferable to be included in
> http://scan.coverity.com/
Oh, we're there, right on Rung 1:
http://scan.coverity.com/rung1.html
But when I click the "Log in" link it gives me a login screen by no way to
create a new account, or request that such an account be associated with the
Subversion results. Their FAQ on the subject is woefully inaccurate:
http://scan.coverity.com/devfaq.html#account
I've tried emailing the generic support account, but I haven't gotten a
response. Maybe I'm just missing something...
-Hyrum
Re: NULL-pointer access when merging with 1.5.1
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Bernhard Merkle wrote:
> Heh. I've been trying for *months* to get access to those results, but to
>> no avail. If Coverity is scanning our source code, it sure doesn't do us
>> much good to not have the results of the scan.
>>
>
> Have you submitted it for inclusion on http://scan.coverity.com/
> or did they do this internally and give you the result ?
>
> As open source project, it is preferable to be included in
> http://scan.coverity.com/
Oh, we're there, right on Rung 1:
http://scan.coverity.com/rung1.html
But when I click the "Log in" link it gives me a login screen by no way to
create a new account, or request that such an account be associated with the
Subversion results. Their FAQ on the subject is woefully inaccurate:
http://scan.coverity.com/devfaq.html#account
I've tried emailing the generic support account, but I haven't gotten a
response. Maybe I'm just missing something...
-Hyrum
Re: NULL-pointer access when merging with 1.5.1
Posted by Bernhard Merkle <be...@googlemail.com>.
Heh. I've been trying for *months* to get access to those results, but to
> no avail. If Coverity is scanning our source code, it sure doesn't do us
> much good to not have the results of the scan.
>
Have you submitted it for inclusion on http://scan.coverity.com/
or did they do this internally and give you the result ?
As open source project, it is preferable to be included in
http://scan.coverity.com/
Bernhard
Re: NULL-pointer access when merging with 1.5.1
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Bernhard Merkle wrote:
> On Mon, Jul 28, 2008 at 4:33 PM, Stefan Küng <to...@gmail.com> wrote:
>
>> Hi,
>>
>> The following conditions will trigger a NULL-pointer access:
>> * svn library 1.5.1 (I guess 1.5.0 too)
>> * merge results in a conflict
>> * user provides his own 'resolved' file in the conflict resolver callback
>> * the users own 'resolved' file has a path *not* in the same folder than
>> the conflicted file is located (best use a completely different path)
>>
>
> I thought SVN is also checked with Coverity (http://scan.coverity.com) ?
>
> Coverity should find NULL pointers so I wonder if the current svn codebase
> is not checked
> or if Coverity does not find that specific case of NULL pointer.
Heh. I've been trying for *months* to get access to those results, but to no
avail. If Coverity is scanning our source code, it sure doesn't do us much good
to not have the results of the scan.
-Hyrum
Re: NULL-pointer access when merging with 1.5.1
Posted by Bernhard Merkle <be...@googlemail.com>.
On Mon, Jul 28, 2008 at 4:33 PM, Stefan Küng <to...@gmail.com> wrote:
> Hi,
>
> The following conditions will trigger a NULL-pointer access:
> * svn library 1.5.1 (I guess 1.5.0 too)
> * merge results in a conflict
> * user provides his own 'resolved' file in the conflict resolver callback
> * the users own 'resolved' file has a path *not* in the same folder than
> the conflicted file is located (best use a completely different path)
>
I thought SVN is also checked with Coverity (http://scan.coverity.com) ?
Coverity should find NULL pointers so I wonder if the current svn codebase
is not checked
or if Coverity does not find that specific case of NULL pointer.
thanks,
Bernhard.
Re: NULL-pointer access when merging with 1.5.1
Posted by Stefan Küng <to...@gmail.com>.
Julian Foad wrote:
> On Mon, 2008-07-28 at 16:33 +0200, Stefan Küng wrote:
>> The following conditions will trigger a NULL-pointer access:
>> * svn library 1.5.1 (I guess 1.5.0 too)
>> * merge results in a conflict
>> * user provides his own 'resolved' file in the conflict resolver callback
>> * the users own 'resolved' file has a path *not* in the same folder than
>> the conflicted file is located (best use a completely different path)
>>
>> Since TortoiseSVN always creates the 'resolved' file in the %TEMP%
>> folder, every attempt to resolve a merge conflict in the callback leads
>> to a segfault.
>>
>> The reason for this:
>>
>> subversion\libsvn_wc\log.c : svn_wc__loggy_copy()
>> calls the function loggy_path() with those paths. But in loggy_path(),
>> if the path to the resolved file is not a child of the folder where the
>> conflict happened, it returns a NULL pointer as the string.
>
> Thanks for the details.
>
>> It seems that loggy_path() is a dangerous function: it assumes that
>> paths are relative to each other. For the 'resolved' file path, this
>> already leads to a segfault. But I assume that this causes other
>> problems with TSVN which I haven't been able yet to determine the exact
>> reason. Remember: UI applications don't have the 'current working
>> directory' set to what the command line client has it set. In TSVN, I
>> had to set the CWD to the %TEMP% folder because some svn functions write
>> temp files to the CWD - and if I don't set the CWD it can be anything,
>> even the SYSTEM32 folder which is write protected.
>
> According to subversion/libsvn_wc/README, a WC action log can refer only
> to files under the working directory that it's operating on. Therefore
> the "loggy_path()" function is never expected to return NULL. Files
> coming in from outside should have first been copied into the admin temp
> directory by the higher level function responsible.
>
> (We could sanity-check the results of loggy_path with SVN_ERR_ASSERT to
> enable more graceful failures from this sort of bug. Would you like to
> make a patch to do that? There's a couple of ways it could be done.)
But that wouldn't fix the problem?
> In this case, it looks like some part of the huge
> svn_wc__merge_internal() is responsible for providing the temporary
> file. There are two parts that look likely:
>
>> SVN_ERR(conflict_func(&result, &cdesc, conflict_baton, pool));
>> [...]
>>
>> if (result->save_merged)
>> {
>> const char *edited_copy;
>> /* ### Should use preserved-conflict-file-exts. */
>> SVN_ERR(svn_io_open_unique_file2(NULL,
>> &edited_copy,
>> merge_target,
>> ".edited",
>> svn_io_file_del_none,
>> pool));
>> SVN_ERR(svn_wc__loggy_copy
>> (log_accum, NULL, adm_access,
>> svn_wc__copy_translate,
>> /* Look for callback's own merged-file first: */
>> result->merged_file
>> ? result->merged_file : result_target,
>> edited_copy,
>> FALSE, pool));
>> }
The code here explicitely checks whether the callback provided its own
merged-file. So I think I'm not doing something illegal by providing my
own file here.
The reason I'm providing my own merged-file instead of using the
provided one is because I found situations where no merged-file was
provided to the callback (i.e, that string was NULL). So instead of
checking the file first and only provide my own if there is none, I
simply create my own every time.
>> switch (result->choice)
>> {
>> [...]
>> /* For the case of 3-way file merging, we don't
>> really distinguish between these return values;
>> if the callback claims to have "generally
>> resolved" the situation, we still interpret
>> that as "OK, we'll assume the merged version is
>> good to use". */
>> case svn_wc_conflict_choose_merged:
>> {
>> SVN_ERR(svn_wc__loggy_copy
>> (log_accum, NULL, adm_access,
>> svn_wc__copy_translate,
>> /* Look for callback's own merged-file first: */
>> result->merged_file ?
>> result->merged_file : result_target,
>> merge_target,
>> FALSE, pool));
>> *merge_outcome = svn_wc_merge_merged;
>> contains_conflicts = FALSE;
>> goto merge_complete;
>> }
>
> I'm guessing one or both of those should be taking a copy of the file by
> normal (non-loggy) means, either instead of or before using a loggy
> copy.
>
> I can't really take this further at the moment. I hope this is useful.
I don't know that part of the Subversion code enough to provide a patch.
But I'd really appreciate a fix for this, as the crash reports coming in
clearly show that this problem is responsible for almost 90% of all the
reports I get.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: NULL-pointer access when merging with 1.5.1
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2008-07-28 at 16:33 +0200, Stefan Küng wrote:
> The following conditions will trigger a NULL-pointer access:
> * svn library 1.5.1 (I guess 1.5.0 too)
> * merge results in a conflict
> * user provides his own 'resolved' file in the conflict resolver callback
> * the users own 'resolved' file has a path *not* in the same folder than
> the conflicted file is located (best use a completely different path)
>
> Since TortoiseSVN always creates the 'resolved' file in the %TEMP%
> folder, every attempt to resolve a merge conflict in the callback leads
> to a segfault.
>
> The reason for this:
>
> subversion\libsvn_wc\log.c : svn_wc__loggy_copy()
> calls the function loggy_path() with those paths. But in loggy_path(),
> if the path to the resolved file is not a child of the folder where the
> conflict happened, it returns a NULL pointer as the string.
Thanks for the details.
> It seems that loggy_path() is a dangerous function: it assumes that
> paths are relative to each other. For the 'resolved' file path, this
> already leads to a segfault. But I assume that this causes other
> problems with TSVN which I haven't been able yet to determine the exact
> reason. Remember: UI applications don't have the 'current working
> directory' set to what the command line client has it set. In TSVN, I
> had to set the CWD to the %TEMP% folder because some svn functions write
> temp files to the CWD - and if I don't set the CWD it can be anything,
> even the SYSTEM32 folder which is write protected.
According to subversion/libsvn_wc/README, a WC action log can refer only
to files under the working directory that it's operating on. Therefore
the "loggy_path()" function is never expected to return NULL. Files
coming in from outside should have first been copied into the admin temp
directory by the higher level function responsible.
(We could sanity-check the results of loggy_path with SVN_ERR_ASSERT to
enable more graceful failures from this sort of bug. Would you like to
make a patch to do that? There's a couple of ways it could be done.)
In this case, it looks like some part of the huge
svn_wc__merge_internal() is responsible for providing the temporary
file. There are two parts that look likely:
> SVN_ERR(conflict_func(&result, &cdesc, conflict_baton, pool));
> [...]
>
> if (result->save_merged)
> {
> const char *edited_copy;
> /* ### Should use preserved-conflict-file-exts. */
> SVN_ERR(svn_io_open_unique_file2(NULL,
> &edited_copy,
> merge_target,
> ".edited",
> svn_io_file_del_none,
> pool));
> SVN_ERR(svn_wc__loggy_copy
> (log_accum, NULL, adm_access,
> svn_wc__copy_translate,
> /* Look for callback's own merged-file first: */
> result->merged_file
> ? result->merged_file : result_target,
> edited_copy,
> FALSE, pool));
> }
>
> switch (result->choice)
> {
> [...]
> /* For the case of 3-way file merging, we don't
> really distinguish between these return values;
> if the callback claims to have "generally
> resolved" the situation, we still interpret
> that as "OK, we'll assume the merged version is
> good to use". */
> case svn_wc_conflict_choose_merged:
> {
> SVN_ERR(svn_wc__loggy_copy
> (log_accum, NULL, adm_access,
> svn_wc__copy_translate,
> /* Look for callback's own merged-file first: */
> result->merged_file ?
> result->merged_file : result_target,
> merge_target,
> FALSE, pool));
> *merge_outcome = svn_wc_merge_merged;
> contains_conflicts = FALSE;
> goto merge_complete;
> }
I'm guessing one or both of those should be taking a copy of the file by
normal (non-loggy) means, either instead of or before using a loggy
copy.
I can't really take this further at the moment. I hope this is useful.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org