Review for a lists patch

From: J.M. Maurer <uwog_at_uwog.net>
Date: Fri Nov 20 2009 - 16:46:37 CET

Say you have some hypothetical list definitions like this:

<lists>
    <l id="1001"/>
    <l id="1002" parentid="1001"/>
    <l id="1003" parentid="1001"/>
</lists>

And also say you never ever use list 1001 in your document. This is
silly of course, but in theory it could be valid. AbiWord itself will
never write that out, but I actually came across generated documents
like that :)

Abiword will crash on this construct (valgrind trace attached):

1. PD_Document::updateDirtyLists will be called
2. It will see the first list is empty:

       if(pAutoNum->isEmpty())

   and it will delete it.
3. Then it will updat the other dirty lists a few lines below:

       pAutoNum->update(0);

=> This will crash in fl_AutoNum::update() on the following line:

       if (m_pParent && !m_pParent->isUpdating())

   as this list's parent list has just been deleted.

The attached proposed patch fixes this by simply not deleting the list,
and not updating a list if it is empty. Since I'm always scared touching
list code, I'd appreciate a review ;)

Cheers!
  Marc

Note: Sometimes AbiWord will NOT crash on this and actually do what you
expect (valgrind will complain loudly though: ). The attached patch will
then look like a regression, since you will now see something like
".6." (the parent has no list number, since it is empty). I find this
behavior way better than crashing, and the "regression" can be easily
fixed by simple setting the parentid to 0.

==16647== Memcheck, a memory error detector.
==16647== Copyright (C) 2002-2007, and GNU GPL\'d, by Julian Seward et al.
==16647== Using LibVEX rev 1804, a library for dynamic binary translation.
==16647== Copyright (C) 2004-2007, and GNU GPL\'d, by OpenWorks LLP.
==16647== Using valgrind-3.3.0, a dynamic binary instrumentation framework.
==16647== Copyright (C) 2000-2007, and GNU GPL\'d, by Julian Seward et al.
==16647== For more details, rerun with: -v
==16647==
==16647== My PID = 16647, parent PID = 16645. Prog and args are:
==16647== /usr/bin/abiword
==16647== --plugin
==16647== AbiCommand
==16647==
==16647== Invalid read of size 1
==16647== at 0x353D1A1B0D: fl_AutoNum::update(unsigned) (fl_AutoNum.cpp:1125)
==16647== by 0x353D26521C: PD_Document::updateDirtyLists() (pd_Document.cpp:5889)
==16647== by 0x353D35AA7E: IE_Exp_Cairo::_writeDocument() (ie_exp_PDF.cpp:127)
==16647== by 0x353D361F1B: IE_Exp::writeFile(char const*) (ie_exp.cpp:325)
==16647== by 0x353D26AEAA: PD_Document::_saveAs(char const*, int, bool, char const*) (pd_Document.cpp:1049)
==16647== by 0x353D3F773C: AD_Document::saveAs(char const*, int, char const*) (xad_Document.cpp:1282)
==16647== by 0x353D2DF077: AP_Convert::convertTo(char const*, int, char const*, int) (ap_Convert.cpp:282)
==16647== by 0x11B28ADB: AbiCommand::parseTokens(UT_GenericVector<UT_UTF8String const*>*) (AbiCommand.cpp:624)
==16647== by 0x11B29406: AbiCommand::doCommands() (AbiCommand.cpp:275)
==16647== by 0x11B2977D: _ZL17AbiCommand_invokeP7AV_ViewP21EV_EditMethodCallData (AbiCommand.cpp:170)
==16647== by 0x353D457D39: ev_EditMethod_invoke(EV_EditMethod const*, UT_String const&) (ev_EditMethod.cpp:294)
==16647== by 0x353D2DE574: AP_App::openCmdLinePlugins(AP_Args const*, bool&) (ap_App.cpp:186)
==16647== Address 0x4f604ee is 78 bytes inside a block of size 336 free\'d
==16647== at 0x4A05B9D: operator delete(void*) (vg_replace_malloc.c:342)
==16647== by 0x353D26517C: PD_Document::updateDirtyLists() (pd_Document.cpp:5878)
==16647== by 0x353D35AA7E: IE_Exp_Cairo::_writeDocument() (ie_exp_PDF.cpp:127)
==16647== by 0x353D361F1B: IE_Exp::writeFile(char const*) (ie_exp.cpp:325)
==16647== by 0x353D26AEAA: PD_Document::_saveAs(char const*, int, bool, char const*) (pd_Document.cpp:1049)
==16647== by 0x353D3F773C: AD_Document::saveAs(char const*, int, char const*) (xad_Document.cpp:1282)
==16647== by 0x353D2DF077: AP_Convert::convertTo(char const*, int, char const*, int) (ap_Convert.cpp:282)
==16647== by 0x11B28ADB: AbiCommand::parseTokens(UT_GenericVector<UT_UTF8String const*>*) (AbiCommand.cpp:624)
==16647== by 0x11B29406: AbiCommand::doCommands() (AbiCommand.cpp:275)
==16647== by 0x11B2977D: _ZL17AbiCommand_invokeP7AV_ViewP21EV_EditMethodCallData (AbiCommand.cpp:170)
==16647== by 0x353D457D39: ev_EditMethod_invoke(EV_EditMethod const*, UT_String const&) (ev_EditMethod.cpp:294)
==16647== by 0x353D2DE574: AP_App::openCmdLinePlugins(AP_Args const*, bool&) (ap_App.cpp:186)
==16647==
==16647== Invalid read of size 4
==16647== at 0x353D1A1B13: fl_AutoNum::update(unsigned) (ut_vector.h:396)
==16647== by 0x353D26521C: PD_Document::updateDirtyLists() (pd_Document.cpp:5889)
==16647== by 0x353D35AA7E: IE_Exp_Cairo::_writeDocument() (ie_exp_PDF.cpp:127)
==16647== by 0x353D361F1B: IE_Exp::writeFile(char const*) (ie_exp.cpp:325)
==16647== by 0x353D26AEAA: PD_Document::_saveAs(char const*, int, bool, char const*) (pd_Document.cpp:1049)
==16647== by 0x353D3F773C: AD_Document::saveAs(char const*, int, char const*) (xad_Document.cpp:1282)
==16647== by 0x353D2DF077: AP_Convert::convertTo(char const*, int, char const*, int) (ap_Convert.cpp:282)
==16647== by 0x11B28ADB: AbiCommand::parseTokens(UT_GenericVector<UT_UTF8String const*>*) (AbiCommand.cpp:624)
==16647== by 0x11B29406: AbiCommand::doCommands() (AbiCommand.cpp:275)
==16647== by 0x11B2977D: _ZL17AbiCommand_invokeP7AV_ViewP21EV_EditMethodCallData (AbiCommand.cpp:170)
==16647== by 0x353D457D39: ev_EditMethod_invoke(EV_EditMethod const*, UT_String const&) (ev_EditMethod.cpp:294)
==16647== by 0x353D2DE574: AP_App::openCmdLinePlugins(AP_Args const*, bool&) (ap_App.cpp:186)
==16647== Address 0x4f604b8 is 24 bytes inside a block of size 336 free\'d
==16647== at 0x4A05B9D: operator delete(void*) (vg_replace_malloc.c:342)
==16647== by 0x353D26517C: PD_Document::updateDirtyLists() (pd_Document.cpp:5878)

Received on Fri Nov 20 16:47:28 2009

This archive was generated by hypermail 2.1.8 : Fri Nov 20 2009 - 16:47:28 CET