Adding keywords

Julian Seward hisself sent me a valgrind log from issue
14425 made with a debugging copy of OOo1.1rc so it has source code
line numbers.  That might help anyone trying to analyze this
without running valgrind themselves.  It's at 
http://kegel.com/openoffice/jseward-14425.txt

I'm raising the priority of this to P2 since it seems to be
implicated in two real-world crashes.

Both are sxi-files. So I set this task to Christian.

send to the developer

evaluating this

set target to 2.0, because the stacks are not showing the root cause
of the crash, however we will investigate it further.

Thorsten, can you please verify this with valgrind on linux? I had a
look at the code and think its a false alarm, but we have to be sure

Okay, I'll take a look.

The invalid memory access in question is a size-2 read,   
of an address 8 bytes before the start of some block.  
This happens at at xmlprmap.hxx line 185:  
  
   /** returns the entry context id */  
   const sal_Int16 GetEntryContextId( sal_Int32 nIndex ) const  
   {  
     return aMapEntries[nIndex].nContextId;  <-- line 185  
   }  
  
Can someone who knows the code add an assertion that nIndex  
is valid at this point?  I think it's highly likely that  
nIndex is out of range (-1, perhaps) and that's what's giving  
rise to the invalid read.  This causes garbage to be read from  
memory and returned to the caller (some kind of search loop  
at XMLShapeStyleContext.cxx:188).  
 

Hi Julian, Thorsten,

I think Julian is probably right here, it looks like an 'array index
out of bounds' error. With the used container, std::vector<>, instead
of 'std::vector<>::operator[]()' (index access) assertion code could
use 'std::vector<>::at()' throwing a 'std::out_of_range' exception:

Thus, instead of

  return aMapEntries[nIndex].nContextId;

one could write:

try
{
  return aMapEntries.at(nIndex).nContextId;
}
catch (std::out_of_range)
{
  assert (0);
  return <invalid_context_id_whatever_that_may_be>;
}

Don't know whether it's easier to leave out the try-catch block and
just wait for an unhandled / unexpected exception to terminate the
process. The 'risk' is in someone silently catching this exception
away so that it may go unnoticed.

Hope that helps,
Matthias

Julian, Matthias, 

thanks for tracking that down. But I think I'll just assert the 
error condition here, don't know if calling code is even exception-aware.

Hi Thorsten,

Well, my code proposal was only meant for bug hunting (assertions), not
for production code. As I understand this case, we have two bugdocs that
trigger this issue, so it should be possible (for testing purposes) to
understand and verify what goes wrong here.

IMHO, the root cause seems to be in a design that requires the
contents of two containers to be in sync all the time, where one
container holds indices into another one. But of course that's only a
guess.

ATM, I'm more interested in whether 'valgrind's complaints can be
verified, so that we either can develop trust in 'valgrind' (and don't
argue about each issue it reports) or give Julian a change to improve
his tool. Having already verified a few issues, my current guess is
that it's the former case.

Matthias

If the OOo developer team decides it trusts Valgrind,
it would probably make sense to clear out a few of the most
frequently occurring Valgrind errors before release of
OOo1.1.  That'll make Valgrind logs easier to wade throgh,
and might even fix some user-visible bugs...

Christian, looks like this is for real. Please take over again.

Will try to reproduce it, if not I just leave the assertion in

Hi Christian,
if you have any trouble reproducing using Valgrind, please
let me know.

BTW last I checked, the crash reporter didn't handle assertions
properly...

Removed nonsense dependency (issue 21786 depending on this one).

The root cause for this where some loop inside xmloff that iterate over a set of
XMLPropertyState and then asked via GetEntryContextId() for a possible context
id and used the mnIndex member from the XMLPropertyState as an index. But the
mnIndex member is also used with a -1 to mark a XMLPropertyState that is now
invalid. So I added a skip into all loops for this case.

I also made the code more failsafe and added the assertion "illegal access to
invalid entry!" in case someone uses an invalid index in GetEntryContextId(),
GetEntryAPIName(),  GetEntryXMLName() , GetEntryNameSpace(), GetEntryType() and
GetEntryFlags()

Fabulous.  Is the fix safe enough to backport to OOo 1.1.1 or 1.1.2?

In theorie it is safe, but if we plan to implement this fix in OOo 1.1.2, I
would suggest just to add the failsafe modification to GetEntryContextId() that
silently returns a 0 when called with the common invalid index -1. I would not
touch any of the faulty loops for a patch.

For testing, please export a wide range of documents and make sure the new
assertion "illegal access to invalid entry!" does not pop up.
Since this is no plattform dependend fix, its ok to just check it under the
build windows nonpro

Set to fixed.

Verified in CWS.

Tested in master. Closed.