Stacked Bugs

ZCodec::ReadAsynchron was as bad as it could get. It took an SvStream instance by reference (rIStm) and did a handful of calls on that. But, sometimes, it would also remember that IStm in a class member variable ZCodec::mpIStm. And it would make calls to mpIStm too, in addition to the calls to rIStm.

The only call to ZCodec::ReadAsynchron is in PNGReaderImpl::ImplReadIDAT, which would pass in stack-allocated SvMemoryStream instances. There can be multiple calls to ZCodec::ReadAsynchron from a while loop in PNGReaderImpl::ImplReadIDAT (all using the same stack-allocated SvMemoryStream instance, aIStrm), but there can also be successive calls to ZCodec::ReadAsynchron itself, thus making calls to ZCodec::ReadAsynchron passing in varying SvMemoryStream instances.

Little surprise, then, that one ZCodec::ReadAsynchron invocation would store away a reference to a stack-allocated SvMemoryStream instance in ZCodec::mpIStm and wrongly reference that in a later ZCodec::ReadAsynchron call, after the SvMemoryStream instance had already gone out of scoped and been destroyed. AddressSanitizer‘s cool UseAfterReturn checker (enabled via ASAN_OPTIONS=detect_stack_use_after_return=1) found that bug now, triggered by one of LibreOffice’s make check tests.

Somewhat surprising, though, that that bug had gone unnoticed for well over a decade, and that tools like Valgrind had never spotted it.

Turns out, the sucessive calls to ZCodec::ReadAsynchron from successive calls to PNGReaderImpl::ImplReadIDAT had always been done at the exact same stack depth. So when ZCodec::ReadAsynchron referenced a dead SvMemoryStream instance through its dangling ZCodec::mpIStm pointer, what actually happened was that it reference the current PNGReaderImpl::ImplReadIDAT‘s SvMemoryStream instance (that it could just as well reference via the passed-in rIStm reference).

And that was apparently what the function wanted to do anyway. A little typo of “mpIStm->Read” instead of “rIStm.Read”, effectively without consequences and well hidden all those years, waiting only for AddressSanitizer to come along and dig it out.


Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s