crash on 1.9.3 (upon merging)

[Stefan <lu...@gmx.de>]

Hi,

I've investigated the crash dump provided on the users mailing list in 
the thread: "Subversion crash report".

Not sure what exactly the underlying issue is, but doing a code review 
suggests that SVN is making use of a nullptr without properly checking 
that case.
starting in libsvn_wc/conflicts.c: build_text_conflict_resolve_items():
1. The call to svn_wc__conflict_read_text_conflict() sets mine_abspath 
to null without returning an error (I assume this is correct behavior 
here). Therefore, it looks like m->is_atom is false in this case (inside 
svn_wc__conflict_read_text_conflict()).
2. If the user chooses "svn_wc_conflict_choose_theirs_conflict" or 
"svn_wc_conflict_choose_mine_conflict" that null-ptr is being passed on 
to merge_showing_conflicts().
3. merge_showing_conflicts() passes that then on to 
svn_diff_file_diff3_2(), then to svn_diff_diff3_2() (in 
baton.files[1].path), then to vtable->datasources_open() (in the 
diff_baton), opens it with svn_io_file_open() where cstring_from_utf8() 
doesn't trigger an error on Windows/Mac (see other thread).

I take it that build_text_conflict_resolve_items() is missing a 
null-check for mine_abspath() in the switch case before calling 
merge_showing_conflicts() (according to what that function does, if 
install_from_abspath is null), but then I take it that you are more 
familiar with the intended behavior here (maybe the missing null-check 
on Windows would be sufficient to restore the originally intended 
behavior?).

Regards,
Stefan

[PATCH] error handling for build_text_conflict_resolve_items

[Stefan <lu...@posteo.de>]

Hi,

> Hi,
>
> I've investigated the crash dump provided on the users mailing list in 
> the thread: "Subversion crash report".
>
> Not sure what exactly the underlying issue is, but doing a code review 
> suggests that SVN is making use of a nullptr without properly checking 
> that case.
> starting in libsvn_wc/conflicts.c: build_text_conflict_resolve_items():
> 1. The call to svn_wc__conflict_read_text_conflict() sets mine_abspath 
> to null without returning an error (I assume this is correct behavior 
> here). Therefore, it looks like m->is_atom is false in this case 
> (inside svn_wc__conflict_read_text_conflict()).
Looking deeper in the code, suggests that this means that there simply 
was no mine_abspath in this case to resolve (looking at the added the 
corresponding data: svn_wc__conflict_skel_add_text_conflict()).

> [...]
> I take it that build_text_conflict_resolve_items() is missing a 
> null-check for mine_abspath() in the switch case before calling 
> merge_showing_conflicts() (according to what that function does, if 
> install_from_abspath is null), but then I take it that you are more 
> familiar with the intended behavior here (maybe the missing null-check 
> on Windows would be sufficient to restore the originally intended 
> behavior?).
Hence I take it that the original conclusion was correct and there 
should simply be an error check being added to 
build_text_conflict_resolve_items().

Attached is a suggested patch with that added error check (it's fully 
untested, since I've got no idea how that case is triggered).
[[
Prevent user reported crash upon conflict resolution with missing 
mine_abspath.
See: 
http://mail-archives.apache.org/mod_mbox/subversion-users/201601.mbox/%3C080F6E74ACB8E84E9FC333A7AB0BFFFB7AF7C020%40OIT-TEAQEXMBX01.som.w2k.state.me.us%3E

Reported by: James Patten <Ja...@maine.gov>

* libsvn_wc/conflicts.c:
    (build_text_conflict_resolve_items): check mine_abspath for null before
                                         using it and return an error 
instead.
]]

Regards,
Stefan