Re: NULL-pointer access when merging with 1.5.1

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