[SyncEvolution] libopenobex 1.7.2 regression (was: Re: Sync on Debian Stretch)
Patrick Ohly
patrick.ohly at intel.com
Tue Sep 5 09:12:42 UTC 2017
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 at 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 at 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 at 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 at 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.
_______________________________________________
SyncEvolution mailing list
SyncEvolution at syncevolution.org
https://lists.syncevolution.org/mailman/listinfo/syncevolution
More information about the SyncEvolution
mailing list