I'm not sure I did all correctly

export LD_LIBRARY_PATH=/data-backup/DEVELOPMENT/Projects/tde-sup/sync_debug/openobex-1.7.2-Source/debian/build/lib

SYNCEVOLUTION_DEBUG=10 ./src/syncevolution --daemon=no -r loglevel=6 nokia_N9 addressbook                                                                  


DEBUG 00:00:02] ready to sync
[DEBUG 00:00:02] starting SAN 12 auth 1B2M2Y8AsgTpgAmY7PhCfg== nonce SyncEvolution session 1 server PC Suite
[DEBUG 00:00:02] SAN datastore addressbook uri Contacts type 7 mode 206
[DEVELOPER 00:00:02] ObexTransportAgent::wait(no reply)
[DEVELOPER 00:00:02] ObexTransportAgent::wait(): iteration
...
[same removed]
...
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): iteration
[DEVELOPER 00:00:04] Connecting Bluetooth device with address 40:98:4E:90:56:E3 and channel 25
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): iteration
[DEVELOPER 00:00:04] OBEX_EV_PROGRESS
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): iteration
[DEVELOPER 00:00:04] OBEX_EV_REQDONE
[DEVELOPER 00:00:04] OBEX Transport: get header who from connect response with value SYNCML-SYNC
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): is ready
[INFO 00:00:04] Server sending SAN
[DEVELOPER 00:00:04] ObexTransport send is called (46 bytes)
[DEVELOPER 00:00:04] ObexTransportAgent::wait(reply)
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): iteration
[DEVELOPER 00:00:04] OBEX_EV_PROGRESS
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): iteration
[DEVELOPER 00:00:04] OBEX_EV_REQDONE
[DEVELOPER 00:00:04] ObexTransport send completed
[DEVELOPER 00:00:04] ObexTransportAgent::wait(): is ready
[DEVELOPER 00:00:04] OBEX_EV_PROGRESS
[DEVELOPER 00:00:04] OBEX_EV_REQDONE
[ERROR 00:00:04] OBEX Request 3 got a failed response Unknown response
[DEBUG 00:00:04] Server Alerted Sync init with SANFormat 12 failed, trying with legacy format
[DEBUG 00:00:04] starting SAN 11 auth 1B2M2Y8AsgTpgAmY7PhCfg== nonce SyncEvolution session 1 server PC Suite
[DEBUG 00:00:04] SAN datastore addressbook uri Contacts type text/x-vcard
[DEBUG 00:00:04] SAN with overall sync mode 206
[kcrash] TDECrash: Application 'SyncEvolution' crashing...
Segmentation fault

In GDB

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff2614358 in smlLibMemcpy () from /usr/lib/x86_64-linux-gnu/libsmltk.so.0
(gdb)  


Thanks in advance for taking your time

regards


On Tue, Sep 5, 2017 at 11:12 AM, Patrick Ohly <patrick.ohly@intel.com> wrote:
On Mon, 2017-09-04 at 20:24 +0200, deloptes wrote:
> Patrick Ohly wrote:
>
> > So have you built 1.5.2 with libopenobex2 from Debian Stretch and
> > it
> > failed? With which phone?
> >
>
> yes - built 1.5.2 with libopenobex2 from Debian Stretch.
> phone is Nokia N9

Unfortunately I indeed don't have that phone.

> > What I did is a "configure --disable-shared --enable-static
> > CFLAGS=-g
> > CXXFLAGS=-g ..." and then in the build directory I ran
> > "SYNCEVOLUTION_DEBUG=10 ./src/syncevolution --daemon=no nokia-n97-
> > mini@
> > devices  addressbook".
> >
> > This allows running the command under gdb. The interesting line is
> > is
> > the error message in ObexTransportAgent::obex_callback. What value
> > does
> > obex_rsp have, and why does OBEX_ResponseToString not know it?
>
> I may try test this next and post the output.

Please do.

I continued debugging this a bit further by running syncevolution under
valgrind. When using libopenobex2, I get:

==15128== Conditional jump or move depends on uninitialised value(s)
==15128==    at 0x4C31CD6: __memcmp_sse4_1 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15128==    by 0x918502D: obex_hdr_it_equals (obex_hdr.c:298)
==15128==    by 0x918661A: obex_msg_post_prepare (obex_msg.c:87)
==15128==    by 0x918661A: obex_msg_prepare (obex_msg.c:117)
==15128==    by 0x9184096: obex_client_request_tx_prepare (obex_client.c:237)
==15128==    by 0x9183358: OBEX_Request (api.c:512)
==15128==    by 0x10AF304: SyncEvo::ObexTransportAgent::connectReq() (ObexTransportAgent.cpp:237)

I don't get that with the older libopenobex. That further strengthens
the hypothesis that this is a regression in libopenobex - unless of
course they changed the API and SyncEvolution now lacks some changes,
but it doesn't look like that this is the case.

Found it. This patch on top of https://gitlab.com/openobex/mainline
master (no changes since 1.7.2 one year ago) fixes it:

>From b496d1781db9c23c9984fc15108871fe21b5fd0d Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Tue, 5 Sep 2017 10:53:30 +0200
Subject: [PATCH 1/1] obex_hdr.c: avoid reading uninitialized memory in
 obex_hdr_it_equals

valgrind correctly reports that the memcmp() in obex_hdr_it_equals()
depends on uninitialized memory:

==15128== Conditional jump or move depends on uninitialised value(s)
==15128==    at 0x4C31CD6: __memcmp_sse4_1 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15128==    by 0x918502D: obex_hdr_it_equals (obex_hdr.c:298)
==15128==    by 0x918661A: obex_msg_post_prepare (obex_msg.c:87)
==15128==    by 0x918661A: obex_msg_prepare (obex_msg.c:117)
==15128==    by 0x9184096: obex_client_request_tx_prepare (obex_client.c:237)
==15128==    by 0x9183358: OBEX_Request (api.c:512)
==15128==    by 0x10AF304: SyncEvo::ObexTransportAgent::connectReq() (ObexTransportAgent.cpp:237)
...

That's because on x86-64, the iterator struct contains padding which
does not get initialized by obex_hdr_it_init_from():

(gdb) p sizeof(it)
$13 = 16
(gdb) p sizeof(it.is_valid)
$14 = 4

Instead of fixing all places where an iterator might get set up, it
seems safer to limit the comparison to the individual fields. There
are few enough of them that this should not affect performance.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 lib/obex_hdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/obex_hdr.c b/lib/obex_hdr.c
index 48576a3..a660405 100644
--- a/lib/obex_hdr.c
+++ b/lib/obex_hdr.c
@@ -295,5 +295,5 @@ void obex_hdr_it_next(struct obex_hdr_it *it)
 
 int obex_hdr_it_equals(const struct obex_hdr_it *a, const struct obex_hdr_it *b)
 {
-       return a && b && (memcmp(a, b, sizeof(*a)) == 0);
+       return a && b && a->list == b->list && a->is_valid == b->is_valid;
 }
-- 
2.11.0

Deloptes, can you rebuild libopenobex2 with that patch and check
whether it helps? It's enough to do "cmake . && make", then set
LD_LIBRARY_PATH so that it includes the lib directory when invoking
syncevolution, i.e. there's no need to actually install libopenobex.

I do get another valgrind hit in libsynthesis also with the older
libopenobex, but that's later. Here's the fix for that:

>From de5f1c80d180c326d02f431c6c3189f1ca9ae6b3 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Tue, 5 Sep 2017 11:03:44 +0200
Subject: [PATCH 1/1] xltenc.c: fix integer underflow

"len" is unsigned, so subtracting 2 yields a very high value and then
the comparison against n causes memory to be read beyond the end of
the buffer when the buffer is smaller than 2.

Happens in SyncEvolution when sending a SAN message to a phone via
OBEX. In practice this typically had no effect because the
uninitialized memory is unlikely to contain exactly the sequence of
bytes that was checked for.

==28571== Invalid read of size 1
==28571==    at 0x1275989: xltEncPcdata (xltenc.c:1177)
==28571==    by 0x1274DAE: xltEncBlock (xltenc.c:1108)
==28571==    by 0x1272DCD: xltEncBlock (xltenc.c:704)
==28571==    by 0x1272698: xltEncInit (xltenc.c:332)
==28571==    by 0x125ED1B: smlStartMessageExt (mgrcmdbuilder.c:207)
==28571==    by 0x112066B: sysync::TSyncSession::StartMessage(sml_sync_hdr_s*) (syncsession.cpp:5512)
==28571==    by 0x113F4A6: sysync::TEngineSessionDispatch::StartMessage(void*, void*, sml_sync_hdr_s*) (enginesessiondispatch.cpp:125)
==28571==    by 0x11038DC: sysync::smlStartMessageCallback(void*, void*, sml_sync_hdr_s*) (syncappbase.cpp:2394)
==28571==    by 0x125F7C7: mgrProcessStartMessage (mgrcmddispatcher.c:247)
==28571==    by 0x125F552: smlProcessData (mgrcmddispatcher.c:138)
==28571==    by 0x12200EA: sysync::TSyncAgent::ServerProcessingStep(unsigned short&, sysync::TEngineProgressType*) (syncagent.cpp:3289)
==28571==    by 0x121FE2B: sysync::TSyncAgent::ServerSessionStep(unsigned short&, sysync::TEngineProgressType*) (syncagent.cpp:3257)
==28571==    by 0x121F5FE: sysync::TSyncAgent::SessionStep(unsigned short&, sysync::TEngineProgressType*) (syncagent.cpp:3058)
==28571==    by 0x114049D: sysync::TServerEngineInterface::SessionStep(sysync::SessionType*, unsigned short&, sysync::TEngineProgressType*) (enginesessiondispatch.cpp:478)
==28571==    by 0x10DD6FC: sysync::SessionStep(void*, sysync::SessionType*, unsigned short*, sysync::TEngineProgressType*) (engineentry.cpp:88)
==28571==    by 0x10D234F: sysync::TEngineModuleBridge::SessionStep(sysync::SessionType*, unsigned short&, sysync::TEngineProgressType*) (enginemodulebridge.cpp:109)
==28571==    by 0x10CABCC: SyncEvo::SharedEngine::SessionStep(boost::shared_ptr<sysync::SessionType> const&, unsigned short&, sysync::TEngineProgressType*) (SynthesisEngine.cpp:94)
==28571==    by 0xFDB098: SyncEvo::SyncContext::doSync() (SyncContext.cpp:4101)
==28571==    by 0xFD6DDE: SyncEvo::SyncContext::sync(SyncEvo::SyncReport*) (SyncContext.cpp:3445)
==28571==    by 0xE4FD57: SyncEvo::Cmdline::run() (Cmdline.cpp:1726)
==28571==    by 0xC26689: main (syncevolution.cpp:531)
==28571==  Address 0x13733782 is 0 bytes after a block of size 2 alloc'd
==28571==    at 0x4C2BBAF: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28571==    by 0x126A2D0: wbxmlOpaqueToken (xltdecwbxml.c:680)
==28571==    by 0x1269485: _nextTok (xltdecwbxml.c:341)
==28571==    by 0x1264BAB: nextToken (xltdec.c:649)
==28571==    by 0x126572C: buildPCData (xltdec.c:2368)
==28571==    by 0x1264F60: buildSyncHdr (xltdec.c:777)
==28571==    by 0x1264A75: xltDecInit (xltdec.c:474)
==28571==    by 0x125F6FC: mgrProcessStartMessage (mgrcmddispatcher.c:225)
==28571==    by 0x125F552: smlProcessData (mgrcmddispatcher.c:138)
==28571==    by 0x12200EA: sysync::TSyncAgent::ServerProcessingStep(unsigned short&, sysync::TEngineProgressType*) (syncagent.cpp:3289)
...

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 src/syncml_tk/src/sml/xlt/all/xltenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/syncml_tk/src/sml/xlt/all/xltenc.c b/src/syncml_tk/src/sml/xlt/all/xltenc.c
index 46ff6fd..3b74828 100755
--- a/src/syncml_tk/src/sml/xlt/all/xltenc.c
+++ b/src/syncml_tk/src/sml/xlt/all/xltenc.c
@@ -1173,7 +1173,7 @@ Ret_t xltEncPcdata(XltTagID_t tagId, XltRO_t reqOptFlag, const VoidPtr_t pConten
               p=((SmlPcdataPtr_t)pContent)->content;
               len=((SmlPcdataPtr_t)pContent)->length;
               n=0;
-              while (n<len-2) {
+              while (n+2<len) {
                 if (p[n]==']' && p[n+1]==']' && p[n+2]=='>') {
                   // we must substitute "]]>" with "]]>]<![CDATA[]>"
                   // - copy what we have so far (includes ]]>)
-- 
2.11.0

--
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.