The account data was unreferenced once too often, or rather, a suitable ref
count increase was missing. A debug build of glib detects that ("GLib:
g_variant_unref: assertion 'value->ref_count > 0' failed"), but without that
check the code might also crash.
Prevents the wallet backend from crashing SyncEvolution when
enabled. Functionality not really tested, though.
PIM backend had compile problems when enabled.
When a backend is inactive, it is meant to ignore generic types like
"calendar". The idea behind that is that typically users install or
compile just the backends they want, and then ask for the "calendar"
backend using the generic sync templates or instructions.
When adding the TDEPIM calendar backend, that broke because it also
instantiated itself for those terms when active. The other TDEPIM
backends already did this as intended.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
The latest activesyncd contains GSettings schema files
which get installed by the top-level makefile, therefore
we have to install everything.
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
The *Register.cpp files and everything they include must compile
without hard dependencies on header files which are TDE specific,
so add some ifdefs. Compiling with TDE backends disabled broke
because of this.
When enabled (untested!), it is unclear how some of these *Register.cpp
could have worked without including the header file that defines the
class they instantiate. Added the necessary includes.
A closing } was missing (found by cppcheck, which tests all variations
of the code, not just those currently enabled).
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
With the recent change ("Add a systemd user service as a backend for the D-Bus
session services"), activating syncevo-dbus-server via D-Bus will integrate
better with systemd. When auto-starting via the .desktop file, we can do the
same by activating via D-Bus.
We use dbus-send for that, if available. A recent busctl from systemd could
also be used, but for example the one in Debian Jessie is still to old. Better
use dbus-send. Directly starting the binary is used as fallback.
Based on a patch from Simon McVittie.
On systems with a systemd user session and a D-Bus user bus that
uses it for activation, this ensures that syncevo-dbus-server ends
up in its own cgroup, instead of being treated as part of dbus.service.
If org._01.pim.contacts and org.syncevolution are activated in quick
succession, it also prevents a race condition that would make one of
the activations fail, similar to
<https://bugs.freedesktop.org/show_bug.cgi?id=53220> in
telepathy-mission-control.
Apparently there's a race condition in the OBEX transport that causes the
connection to phones via Bluetooth to be shut down prematurely. Some phones
react by doing a slow sync instead of an incremental sync the next time.
Waiting during shutdown should address the problem (however, it was not
possible to confirm this).
syncevolution.org binaries are now getting compiled on Ubuntu Trusty and thus
no longer support distros with older EDS. The code should still compile
against older EDS (for example, for Maemo), but that is not getting tested
anymore.
This allows removing the dynamic linker hacks related to older libraries,
which was only used in those binaries. Instead, backends using libical or EDS
get compiled on Ubuntu Trusty and then the soname of those libs get patched to
make the backend module usable in combination with a different set of
libs. That patching is part of a script maintained in the syncevolution.org
build infrastructure.
This approach was already used before to generate different EDS backends
for EDS versions with the newer EClient API, because that turned out to be
easier than the dynamic loading approach. It works because none of the methods
used by SyncEvolution changed their ABI, only some other parts of the
libraries did. Should there ever be a situation again that cannot be handled
like this, then backends might also get compiled on different distros than
Ubuntu Trusty (however, that may lead to problems due to the libstdc++ ABI
changes - to be decided...).
libical still requires one special hack: system time zone loading in
libical v1 (and only in that version, v2 has builtin support again) must
be overridden such that time zones are generated with rules instead
of transitions because that is more compatible with the peers that
SyncEvolution exchanges data with.
That hack now relies on overriding the two relevant functions inside the main
binaries (has to be there, otherwise libical still ends up calling its own
internal implementation). The overriding code is in
libsyncevo-icaltz-util.so.0 and depends on libical.so.1. If
libsyncevo-icaltz-util.so.0 can be loaded, the wrappers in the main binary use
it, otherwise they fall through to the code from the current libical.so, which
then should be libical.so.2 or more recent.
This hack is active by default when libical v1 is detected during configuration.
Previously, backends were loaded in a constructor. At some point (*), that
loading crashed with a segfault in the dynamic linker. To make debugging a bit
easier and rule out non-determinism as the root cause of that crash, loading
backends was moved into SyncContect::initMain().
(*) the crash happened when there were two backends which couldn't
be loaded due to missing libraries and was related to error strings.
A simpler test program did not trigger the problem, and now SyncEvolution
has suitable backends for (hopefully?!) all platforms, so it no longer
occurs, at least not during automated testing.
SYNCEVOLUTION_DEBUG=1 syncevolution --daemon=no --version now
dumps also the debug information gathered by the binary
compatibility code. It was only available in sync logs before.
We call some pthread methods and thus must link against libpthread.
Previously this must have worked by accident, but started failing
on more recent distros.
Recent shells filter out environment variables that are not valid shell
variables, so for example,
SYNCEVOLUTION_FILE_SOURCE_DELAY_OPEN_addressbook-slow-server did not get
passed through to syncevo-dbus-server because of the hyphen. As a result, the
tests became unreliable (non-deterministic timing) or outright failed.
Now we ensure that these variables are valid also in a shell and in addition,
make the test stricter such that it detects when the file backend did not
wait.
These linker flags are added to those normally used for linking against
libsyncevolution. The intended usage is to extend linking of syncevolution.org
binaries such that static versions of libcppunit, libpcrecpp and libopenobex
are used, because the ABI of those libs have changed such that binaries linked
on Ubuntu Trusty would not run on more recent distros like Ubuntu Xenial.
For example, on Ubuntu Trusty i386 one can configure with:
PCRECPP_LIBS=-lpcre \
LIBOPENOBEX_LIBS=-lpcre \
'--with-extra-core-ldadd=/usr/lib/i386-linux-gnu/libpcrecpp.a -lpcre /usr/lib/libopenobex.a /usr/lib/i386-linux-gnu/libusb.a' \
CPPUNIT_LIBS=/usr/lib/i386-linux-gnu/libcppunit.a
Now cppcheck is found via the normal PKG_CHECK_MODULES. The advantage
is that CPPUNIT_CFLAGS and CPPUNIT_LIBS can be overridden with
configure parameters, which will be used to link cppcheck statically
into syncevolution.org binaries (libcppunit ABI has changed).
syncevolution.org binaries are going to support distros >= Ubuntu Trusty.
All of those have libnotify.so.4, so the old binary compatibility hack is
no longer needed.
cppcheck correctly warned that initializing a reference is enough,
no need to copy the string:
dbus-sync.cpp:97: cppcheck performance: passedByValue - Function parameter
'source' should be passed by reference.
cppcheck seems to mix up different template variations. The following error
needs to be suppressed in the classes which don't even have an m_a member:
gdbus-cxx-bridge.h:330: cppcheck warning: uninitMemberVar - Member variable
'Set::m_a' is not initialized in the constructor.
gdbus-cxx-bridge.h:339: cppcheck warning: uninitMemberVar - Member variable
'Set::m_a' is not initialized in the constructor.
cppcheck correctly warns about the old code as less efficient (looks
at entire string instead of just the start). Using boost::starts_with
is less obscure and avoids the warning.
When compiling as C++14 (the default under gcc 6), template
expansion no longer picks the right instantiation of push_back.
We have to disambiguate which of the different push_back
alternatives we want.
Fixes:
Cmdline.cpp:1734:85: error: no matching function for call to 'bind(<unresolved
overloaded function type>, const boost::reference_wrapper<std::__cxx11::list<std::__cxx11::basic_string<char> >
>, const boost::arg<1>&)'
processLUIDs(source, boost::bind(&list<string>::push_back, boost::ref(luids), _1));
The implicit conversion of boost::shared_ptr->bool is no
longer supported when compiling as C++14 (the default under
gcc 6). We have to cast explicitly.
Fixes errors like this:
Logging.h:258:41: error: cannot convert 'const boost::shared_ptr<SyncEvo::Logger>' to 'bool' in return
operator bool () const { return m_logger; }
The previous mkdir_p() walked down top to bottom and checked each path
entry as it went along. That approach failed unnecessarily when some
existing parent directory could not be read (non-readable /home, for
example).
The new version tries to create the lowest directories first
(brute-force) and only walks up when that is necessary to avoid an
ENOENT.
As reported by Canonical, syncing fails if data items contain
text which is not correct UTF-8 in one of the fields that
SyncEvolution logs in the command line output (like SUMMARY of
a calendar event).
That is because the byte string coming from the item is passed
unchecked to the D-Bus implementation for transmission via D-Bus. But
D-Bus strings must be correct UTF-8, so depending on the D-Bus library
in use, one gets a segfault (GIO D-Bus, due to an unchecked NULL
pointer access) or an "out of memory" error (libdbus, which checks for
NULL).
What the D-Bus bindings now do is checking the string in advance (to
avoid error messages inside the D-Bus implementations) and then
replacing all invalid bytes with question marks. The rest of the
string is preserved.
Handling this inside the D-Bus binding layer is not the "correct"
solution (which would be to check for UTF-8 in the higher layers were
such bad data might get into SyncEvolution), but it is the less
intrusive and more complete one. Changing the bindings such that all
strings must be declared explicitly as "UTF-8 string" would have been
a way to find all places where such checks are missing, but it turned
out to be too complex and requiring too many changes.
Extracting a meaningful description of each item from the Synthesis
engine when updating and adding items is easy to do for items of
certain known types (contacts and calendar items) and arguably an
improvement; in particular it makes tests like
TestCmdline.testSyncOutput more realistic.
In some cases, the prefix which was supposed to be embedded
in the log messages from the target side of a local sync got
lost on the way to the command line tool.
Primarily this affected the added/updated/deleted messages, as in:
[INFO remote@client] @client/addressbook: started
[INFO remote@client] updating "Joan Doe"
[INFO remote@client] @client/addressbook: received 1/1
This was not caught by automated testing because the corresponding
test (TestCmdline.testSyncOutput and TestCmdline.testSyncOutput2) is
using the file backend, which does not generate such messages at the
moment (to be changed separately).
When sending an access token with insufficient scope (for example,
because the Ubuntu Online Accounts service definition was incomplete,
as documented in FDO #86824), Google responds with a 403 "service
denied" error.
Neon (arguably correctly) treats this as a permanent error and not
as a transient authentication error. Google should better send
a 401 error.
To activate the 401 error handling in SyncEvolution, detect this
special case and turn the general SE_ERROR error into SE_AUTH.
The Ubuntu Online Accounts signon backend complained about
invalid "username" content with an error message containing "sso",
the string used for gSSO.
Define these magic strings once and then use only the defines.
Use the information contained in the AgAuthData object to decide which
authentication method is being used.
Move the common authentication code into a private authenticate()
method.
[PO: fixed g_variant_builder_end() mem leak]
[PO: avoid issue found via cppcheck: returning std::string::c_str() in
a method returning a std::string forces a string copy, which is
inefficient.]
[PO: UiPolicy was not set because of an invalid g_variant_builder_add()
"sv" parameter.]
Instead of passing the session data and the mechanism, pass the full
AgAuthData object to the SignonAuthProvider constructor.
This object can be used to build the session data without converting all
dictionaries to and from GHashTable.
[PO: fixed g_variant_builder_end() mem leak]
[PO: ForceTokenRefresh was not set because of an invalid g_variant_builder_add()
"sv" parameter.]
It's not sync-evolution task to complete the account creation; in those
platforms where this backend is deployed (Ubuntu, Elementary, KDE) the
account should already be created with a proper SignonIdentity attached
to it.
Remove the failure count from the getOAuth2Bearer() method, and add an
invalidateCachedSecrets() method instead.
This moves the logic of how to deal with failures back to the
AuthProvider backend and simplifies the session code, which only needs
to call invalidateCachedSecrets() when the token is wrong.
This will help implementing a similar logic for the getCredentials()
method, where authentication errors could lead to requesting a new
password.
[PO: avoid issue found via cppcheck: returning std::string::c_str() in
a method returning a std::string forces a string copy, which is
inefficient.]
The libaccounts-based backends ("gsso" and "uoa") are going to use more
of libaccounts-glib, making sharing of code with the plain "signon"
plugin much harder to maintain.
Since 1.4.99.4, syncing WebDAV collections always checks first
whether there are items in the collections. This was partly done for
slow sync prevention (which is not necessary for empty collections),
partly for the "is the datastore usable" check.
However, this did not take into account that for CalDAV collections,
the entire content gets downloaded for this check. That is because
filtering by item type (VEVENT vs. VJOURNAL) is not implemented
correctly by all servers. So now all CalDAV syncs, whether incremental
or slow, always transfered all items, which is not the
intention (incremental syncs should be fast and efficient).
This commit adds a more efficient isEmpty() check: for simple CardDAV
collections, only luid and etag get transferred, as in
listAllItems(). This is the behavior from 1.5.
For CalDAV, a report with a filter for the content type is used and
the transfer gets aborted after the first item, without actually
double-checking the content of the item. This is different from
listAllItems(), which really transfers the content. This extra content
check would only be needed for some old servers (Radical 0.7) and is
not essential, because reporting "not empty" even when empty is safe.
Capture the item status and pass it to the response handler.
Response handlers are allowed to return a non-zero integer when using
the initAbortingReportParser(), which then aborts processing of the
response.
This leads to errors being returned by
ne_xml_dispatch_request(). Session::run() needs to be told if the
request was aborted, in which case all errors are ignored.
It turned out that finding databases on an Apple Calendar server accessed via
http depends on sending Basic Auth even when the server does not ask for it:
without authentication, there is no information about the current principal,
which is necessary for finding the user's databases.
To make this work again, sending the authentication header is now forced for
plain http if (and only if) the request which should have returned the
principal URL fails to include it. This implies sending the same request
twice, but as this scenario should be rare in practise (was only done for
testing), this is acceptable.
When storing a pointer with PlainGStr::reset(), the default delete []
was used to free the memory. Should have used g_free(), as in the
constructor. Affects the signon backend.
cppcheck warned about the non-portable initialization of a float
via memset. It is okay on all current architectures, but better
assign a real float 0 value anyway, while keeping the memset for
the rest of the elements.
The second memset() had already become redundant when adding the
PullParams constructor.
When the SuspendFlags::handleSignal() signal handler got called while
the program was allocating memory in libc, a deadlock occurred when
the mutex locking code in RecMutex::lock() tried to allocate the guard
instance (see backtrace below).
This could be fixed with new mutex code which gives up the guard
concept, a guard concept using C11 mechanisms (copy semantic of a
stack-allocated guard) or not locking at all in getSuspendFlags().
This commit uses the latter. It is simpler and can still be done
correctly because locking is not necessary except when the singleton
has not been allocated yet. That part gets moved into the process
startup phase via SyncContext::initMain().
Thread 1 (Thread 0x7fec389e6840 (LWP 16592)):
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95
from /lib/x86_64-linux-gnu/libc.so.6
from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
d=<optimized out>, p=0x7fec38196f60 <SyncEvo::suspendRecMutex>,
this=<synthetic pointer>)
at /usr/include/boost/smart_ptr/detail/shared_count.hpp:167
p=0x7fec38196f60 <SyncEvo::suspendRecMutex>, this=<synthetic pointer>)
at /usr/include/boost/smart_ptr/shared_ptr.hpp:363
this=<synthetic pointer>)
at /data/runtests/work/sources/syncevolution/src/syncevo/ThreadSupport.h:60
at /data/runtests/work/sources/syncevolution/src/syncevo/ThreadSupport.h:81
at /data/runtests/work/sources/syncevolution/src/syncevo/SuspendFlags.cpp:58
at /data/runtests/work/sources/syncevolution/src/syncevo/SuspendFlags.cpp:280
oldp=0x115b7d0, oldsize=80, nb=48) at malloc.c:4208
at malloc.c:3029
n_bytes=n_bytes@entry=32)
The class is used for non-GObject types, so we must use a
type-specific method to increase the
ref-count. intrusive_ptr_add_ref() does that automatically for us.
Calling g_object_ref() was a cut-and-paste error.
Jonas Smedegaard
Patrick Ohly
deloptes
SyncEvolution Nightly Testing
Simon McVittie
Niels Ole Salscheider
Alberto Mardegan