libopenobex 1.7.2 regression (was: Re: [SyncEvolution] Sync on Debian Stretch)
deloptes
deloptes at gmail.com
Fri Sep 29 20:22:15 UTC 2017
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 at 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 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.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/syncevolution/attachments/20170929/6400ac38/attachment.htm>
More information about the SyncEvolution
mailing list