Summary: | Crash when importing malformed vcard file | ||
---|---|---|---|
Product: | SyncEvolution | Reporter: | Xavier Claessens <xclaesse> |
Component: | SyncEvolution | Assignee: | SyncEvolution Community <syncevolution-issues> |
Status: | RESOLVED NOTOURBUG | QA Contact: | |
Severity: | normal | ||
Priority: | medium | CC: | syncevolution-issues, xclaesse |
Version: | unspecified | ||
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | |||
i915 platform: | i915 features: | ||
Attachments: |
Minimal vcf file
Stacktrace Patch to skip empty contacts |
Created attachment 96074 [details]
Stacktrace
First, some general comments. The --import operation is not what happens during a normal sync and therefore cannot be used to debug data handling issues encountered during syncing. The difference is that during syncing, the Synthesis engine parses individual items, transforms each one for the storage and only then sends it to the storage. --import on the other hand handles multiple items (separated by blank lines by default) and sends the individual items directly to the backend, without conversion. When using the command line, it is easier for debugging to use --daemon=no, because then the actual execution happens inside the current process instead of getting moved to syncevo-dbus-server. So in this case, --import should indeed not crash, but it also can't be used like this to import directly from the phone. You need to disable splitting at the empty line, like this: syncevolution --daemon=no database=test backend=evolution-contacts --import /tmp/address-lines.vcf --delimiter none That works because EDS happens to support vCard 2.1 in addition to vCard 3.0. Now regarding the crash: this seems be a bug in the EDS vCard parser when passed incomplete items. The first "item" can be imported: $ cat /tmp/address-lines.vcf BEGIN:VCARD VERSION:2.1 FN:My name N:My name pohly@pohly-mobl1:~/work/syncevolution/src$ ./syncevolution --daemon=no database=test backend=evolution-contacts --import /tmp/address-lines.vcf #0: pas-id-532AAB1700000006 pohly@pohly-mobl1:~/work/syncevolution/src$ ./syncevolution --daemon=no database=test backend=evolution-contacts --export - --luids pas-id-532AAB1700000006 BEGIN:VCARD VERSION:3.0 UID:pas-id-532AAB1700000006 FN:My name N:My name REV:2014-03-20T08:47:19Z END:VCARD The second one can't: $ cat /tmp/address-lines.vcf END:VCARD $ GLIBCXX_FORCE_NEW=1 G_SLICE=always-malloc G_DEBUG=gc-friendly SYNCEVOLUTION_DEBUG=1 valgrind --num-callers=20 ./syncevolution --daemon=no database=test backend=evolution-contacts --import /tmp/address-lines.vcf ==29516== Memcheck, a memory error detector ==29516== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al. ==29516== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info ==29516== Command: ./syncevolution --daemon=no database=test backend=evolution-contacts --import /tmp/address-lines.vcf ==29516== [DEBUG 00:00:00] checking password property 'password' in config '@default' with user identity '' [DEBUG 00:00:00] checking password property 'databasePassword' in config '@default' with user identity '' [DEBUG 00:00:01] : slow sync or testing, do full item scan to detect changes [DEVELOPER 00:00:02] libebook: vcard began without a BEGIN:VCARD ==29516== Invalid read of size 8 ==29516== at 0xA70C6B6: e_vcard_get_attribute (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA702019: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0x745FBD3: g_object_set_valist (gobject.c:1352) ==29516== by 0x74603E6: g_object_set (gobject.c:2053) ==29516== by 0xBECF04: SyncEvo::EvolutionContactSource::insertItem(std::string const&, std::string const&, bool) (EvolutionContactSource.cpp:1037) ==29516== by 0xF8612F: boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>::operator()(SyncEvo::TrackingSyncSource*, std::string const&, std::string const&, bool) const (mem_fn_template.hpp:393) ==29516== by 0xF8605E: SyncEvo::SyncSourceRaw::InsertItemResult boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> >::operator()<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list0>(boost::_bi::type<SyncEvo::SyncSourceRaw::InsertItemResult>, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>&, boost::_bi::list0&, long) (bind.hpp:447) ==29516== by 0xF85F82: boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >::operator()() (bind_template.hpp:20) ==29516== by 0xF85C46: boost::detail::function::function_obj_invoker0<boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >, SyncEvo::SyncSourceRaw::InsertItemResult>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:132) ==29516== by 0xDF1624: boost::function0<SyncEvo::SyncSourceRaw::InsertItemResult>::operator()() const (function_template.hpp:759) ==29516== by 0xF84062: SyncEvo::TrackingSyncSource::continueInsertItem(boost::function<SyncEvo::SyncSourceRaw::InsertItemResult ()()> const&, std::string const&) (TrackingSyncSource.cpp:148) ==29516== by 0xF84428: SyncEvo::TrackingSyncSource::doInsertItem(std::string const&, std::string const&, bool) (TrackingSyncSource.cpp:162) ==29516== by 0xF84614: SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:174) ==29516== by 0xF84796: virtual thunk to SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:180) ==29516== by 0xD1E1E3: SyncEvo::Cmdline::insertItem(SyncEvo::SyncSourceRaw*, std::string const&, std::string const&) (Cmdline.cpp:1732) ==29516== by 0xD1614B: SyncEvo::Cmdline::run() (Cmdline.cpp:1510) ==29516== by 0xB7197B: main (syncevolution.cpp:531) ==29516== Address 0x991db08 is 8 bytes inside a block of size 48 free'd ==29516== at 0x4C27D4E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29516== by 0xA70AEC8: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA70C6A3: e_vcard_get_attribute (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA702019: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0x745FBD3: g_object_set_valist (gobject.c:1352) ==29516== by 0x74603E6: g_object_set (gobject.c:2053) ==29516== by 0xBECF04: SyncEvo::EvolutionContactSource::insertItem(std::string const&, std::string const&, bool) (EvolutionContactSource.cpp:1037) ==29516== by 0xF8612F: boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>::operator()(SyncEvo::TrackingSyncSource*, std::string const&, std::string const&, bool) const (mem_fn_template.hpp:393) ==29516== by 0xF8605E: SyncEvo::SyncSourceRaw::InsertItemResult boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> >::operator()<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list0>(boost::_bi::type<SyncEvo::SyncSourceRaw::InsertItemResult>, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>&, boost::_bi::list0&, long) (bind.hpp:447) ==29516== by 0xF85F82: boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >::operator()() (bind_template.hpp:20) ==29516== by 0xF85C46: boost::detail::function::function_obj_invoker0<boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >, SyncEvo::SyncSourceRaw::InsertItemResult>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:132) ==29516== by 0xDF1624: boost::function0<SyncEvo::SyncSourceRaw::InsertItemResult>::operator()() const (function_template.hpp:759) ==29516== by 0xF84062: SyncEvo::TrackingSyncSource::continueInsertItem(boost::function<SyncEvo::SyncSourceRaw::InsertItemResult ()()> const&, std::string const&) (TrackingSyncSource.cpp:148) ==29516== by 0xF84428: SyncEvo::TrackingSyncSource::doInsertItem(std::string const&, std::string const&, bool) (TrackingSyncSource.cpp:162) ==29516== by 0xF84614: SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:174) ==29516== by 0xF84796: virtual thunk to SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:180) ==29516== by 0xD1E1E3: SyncEvo::Cmdline::insertItem(SyncEvo::SyncSourceRaw*, std::string const&, std::string const&) (Cmdline.cpp:1732) ==29516== by 0xD1614B: SyncEvo::Cmdline::run() (Cmdline.cpp:1510) ==29516== by 0xB7197B: main (syncevolution.cpp:531) ==29516== [ERROR 00:00:02] GLib: g_ascii_strcasecmp: assertion `s1 != NULL' failed ==29516== Invalid read of size 8 ==29516== at 0xA709830: e_vcard_attribute_remove_values (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA701D51: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0x745FBD3: g_object_set_valist (gobject.c:1352) ==29516== by 0x74603E6: g_object_set (gobject.c:2053) ==29516== by 0xBECF04: SyncEvo::EvolutionContactSource::insertItem(std::string const&, std::string const&, bool) (EvolutionContactSource.cpp:1037) ==29516== by 0xF8612F: boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>::operator()(SyncEvo::TrackingSyncSource*, std::string const&, std::string const&, bool) const (mem_fn_template.hpp:393) ==29516== by 0xF8605E: SyncEvo::SyncSourceRaw::InsertItemResult boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> >::operator()<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list0>(boost::_bi::type<SyncEvo::SyncSourceRaw::InsertItemResult>, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>&, boost::_bi::list0&, long) (bind.hpp:447) ==29516== by 0xF85F82: boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >::operator()() (bind_template.hpp:20) ==29516== by 0xF85C46: boost::detail::function::function_obj_invoker0<boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >, SyncEvo::SyncSourceRaw::InsertItemResult>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:132) ==29516== by 0xDF1624: boost::function0<SyncEvo::SyncSourceRaw::InsertItemResult>::operator()() const (function_template.hpp:759) ==29516== by 0xF84062: SyncEvo::TrackingSyncSource::continueInsertItem(boost::function<SyncEvo::SyncSourceRaw::InsertItemResult ()()> const&, std::string const&) (TrackingSyncSource.cpp:148) ==29516== by 0xF84428: SyncEvo::TrackingSyncSource::doInsertItem(std::string const&, std::string const&, bool) (TrackingSyncSource.cpp:162) ==29516== by 0xF84614: SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:174) ==29516== by 0xF84796: virtual thunk to SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:180) ==29516== by 0xD1E1E3: SyncEvo::Cmdline::insertItem(SyncEvo::SyncSourceRaw*, std::string const&, std::string const&) (Cmdline.cpp:1732) ==29516== by 0xD1614B: SyncEvo::Cmdline::run() (Cmdline.cpp:1510) ==29516== by 0xB7197B: main (syncevolution.cpp:531) ==29516== Address 0x991db18 is 24 bytes inside a block of size 48 free'd ==29516== at 0x4C27D4E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29516== by 0xA70AEC8: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA70C6A3: e_vcard_get_attribute (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0xA702019: ??? (in /usr/lib/libebook-1.2.so.13.3.1) ==29516== by 0x745FBD3: g_object_set_valist (gobject.c:1352) ==29516== by 0x74603E6: g_object_set (gobject.c:2053) ==29516== by 0xBECF04: SyncEvo::EvolutionContactSource::insertItem(std::string const&, std::string const&, bool) (EvolutionContactSource.cpp:1037) ==29516== by 0xF8612F: boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>::operator()(SyncEvo::TrackingSyncSource*, std::string const&, std::string const&, bool) const (mem_fn_template.hpp:393) ==29516== by 0xF8605E: SyncEvo::SyncSourceRaw::InsertItemResult boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> >::operator()<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list0>(boost::_bi::type<SyncEvo::SyncSourceRaw::InsertItemResult>, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>&, boost::_bi::list0&, long) (bind.hpp:447) ==29516== by 0xF85F82: boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >::operator()() (bind_template.hpp:20) ==29516== by 0xF85C46: boost::detail::function::function_obj_invoker0<boost::_bi::bind_t<SyncEvo::SyncSourceRaw::InsertItemResult, boost::_mfi::mf3<SyncEvo::SyncSourceRaw::InsertItemResult, SyncEvo::TrackingSyncSource, std::string const&, std::string const&, bool>, boost::_bi::list4<boost::_bi::value<SyncEvo::TrackingSyncSource*>, boost::_bi::value<std::string>, boost::_bi::value<std::string>, boost::_bi::value<bool> > >, SyncEvo::SyncSourceRaw::InsertItemResult>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:132) ==29516== by 0xDF1624: boost::function0<SyncEvo::SyncSourceRaw::InsertItemResult>::operator()() const (function_template.hpp:759) ==29516== by 0xF84062: SyncEvo::TrackingSyncSource::continueInsertItem(boost::function<SyncEvo::SyncSourceRaw::InsertItemResult ()()> const&, std::string const&) (TrackingSyncSource.cpp:148) ==29516== by 0xF84428: SyncEvo::TrackingSyncSource::doInsertItem(std::string const&, std::string const&, bool) (TrackingSyncSource.cpp:162) ==29516== by 0xF84614: SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:174) ==29516== by 0xF84796: virtual thunk to SyncEvo::TrackingSyncSource::insertItemRaw(std::string const&, std::string const&) (TrackingSyncSource.cpp:180) ==29516== by 0xD1E1E3: SyncEvo::Cmdline::insertItem(SyncEvo::SyncSourceRaw*, std::string const&, std::string const&) (Cmdline.cpp:1732) ==29516== by 0xD1614B: SyncEvo::Cmdline::run() (Cmdline.cpp:1510) ==29516== by 0xB7197B: main (syncevolution.cpp:531) ... That looks like libebook produces an invalid EContact instance. Do you want to investigate that further or may I close it as "NOTOURBUG"? Regarding syncing with the phone, what issue do you see there? Also a crash? To debug that, run the sync with loglevel=4 and then check the syncevolution-log.html. It should show in great detail how the incoming vCard is handled. Note that empty lines in vCard and iCalendar are dubious. The specs do not cover the case. I just checked it the other day because KDE also inserts an empty line in iCalendar items. The consensus seems to be that parsers should handle it, but generators should avoid it, because some parsers fail on empty lines. Something that needs to be fixed in SyncEvolution is the error handling once syncevo-dbus-helper executes the failing libebook code. I tried it the same way as you did and see a similar deadlock in the SyncEvo::LogRedirect::process method. Thanks for the --daemon=no trick. It is indeed a bug in libebook that I'm hunting now. But I think there is still something wrong in syncevolution. As I understand correctly, it is syncevolution who split the vcf file into individual vcards, right? If I pass the "--delimiter none" option it fix the problem, but then if I put a file with 2 vcards it won't import the 2nd. So if I understand correctly, there are 2 bugs: 1) libebook should deal with vCards missing the END:VCARD without crashing on that [ERROR 00:00:02] GLib: g_ascii_strcasecmp: assertion `s1 != NULL' failed 2) syncevolution should deal with vcf files containing blank lines and split it correctly into individual vcards. Note that strictly speaking, if I understand correctly the RFC[1], blank lines are forbidden between BEGIN:VCARD and END:VCARD, because there can be only one CRLF at the end of each line. But still, parsers should be tolerant, vcard writers are known to never fully respect the RFC. Never seen a single device out there that always produce valid vcards. Never. [1] https://tools.ietf.org/html/rfc6350#section-3.3 (In reply to comment #4) > 2) syncevolution should deal with vcf files containing blank lines and split > it correctly into individual vcards. No, SyncEvolution's --import wasn't designed for that. The code doing the splitting is completely unaware of what kind of items it or the backend deals with. All of the item manipulation operations are thin wrappers around the existing backend methods. That import/export can deal with multiple items is a convenience feature that happens to work for vCard 3.0 and iCalendar 2.0 (at least as long redundant empty lines are avoided). It does not work for vCard 2.1 or vCalendar 1.0 because those formats may contain empty lines (if I remember correctly). This is different from a dedicated "import vcard file" feature. Such a feature would require the backends to implement additional functionality that is not needed for syncing. Therefore it is a bit out of scope for SyncEvolution. Having said that, I would be okay with accepting patches that enhance the feature. I reported a bug on EDS with a patch for the when the vcard is just "END:VCARD": https://bugzilla.gnome.org/show_bug.cgi?id=726788 With that fix, all vcards are actually imported correctly since it won't crash, but it adds lots of empty contacts because it considers "END:VCARD" to be a full vcard. One possible fix for that is skipping empty contacts in EvolutionContactSource::insertItem() ? Created attachment 96122 [details] [review] Patch to skip empty contacts Creating a contact with no properties is unusual, but not an error. The backend shouldn't silently skip over such contacts and return an empty ID. That'll cause sync failures. We could throw an error to indicate that the contact was not okay, but then you don't achieve your goal of having "--import" silently import all contacts when the vcard file is broken. No, I think the only sane solution would be to implement a semantic file parser. Until then, --import should work as currently specified. Right, I understand. The change is more invasive and I probably won't have time to work on that though. I think we have addressed the original problems, the shutdown problem from comment #3. Let's close it. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Created attachment 96073 [details] Minimal vcf file When syncing contacts from a bt phone, syncevolution crashes. This is because the phone is sending malformed vcf file (see attached file). The problem is that in some cases it adds a blank line before the END:VCARD. I think it should be a bit more tolerant on errors. To reproduce, just run the command: syncevolution --import address-lines.vcf backend=evolution-contacts