darlinghq / darling-dmg

FUSE module for .dmg files (containing an HFS+ filesystem)
http://www.darlinghq.org
GNU General Public License v3.0
278 stars 45 forks source link

HFSAttributeBTree::getattr crashes #68

Open tomkoen opened 6 years ago

tomkoen commented 6 years ago
leafNode = findLeafNode((Key*) &key, cnidAttrComparator);

if findLeafNode returns empty HFSBTreeNode then operator= will crash in initFromBuffer. I'd change the method code to

void initFromBuffer()
{
   if (m_descriptorData.size()) // required check!
   {
      m_descriptor = reinterpret_cast<BTNodeDescriptor*>(&m_descriptorData[0]);
      m_firstRecordOffset = reinterpret_cast<uint16_t*>(descPtr() + m_nodeSize - sizeof(uint16_t));
   }
}
jief666 commented 6 years ago

I've just tried and it doesn't crash.

tomkoen commented 6 years ago

Without the check m_descriptorData[0] is trying to access the first element of the vector while the vector is empty.

jief666 commented 6 years ago

Yes, I wrote earlier too quick. Seems that vector index out of bounds is undefined. Clang seems to return NULL, which, by coincidence, works well here.

Must be corrected, yes.

Maybe

    HFSBTreeNode& operator=(const HFSBTreeNode& that)
    {
        if (!that.isInvalid()) {
            m_descriptorData = that.m_descriptorData;
            m_nodeSize = that.m_nodeSize;
            initFromBuffer();
        }else{
            m_descriptor = 0;
            m_nodeSize = 0;
            m_firstRecordOffset = 0;
        }
        return *this;
    }
tomkoen commented 6 years ago

Thanks for confirming. Quick code to reproduce:

HFSBTreeNode retEmptyNode() { return HFSBTreeNode(); }

HFSBTreeNode crashNode;
crashNode = retEmptyNode(); // oops
jief666 commented 6 years ago

Which platform/compiler do you use ?

tomkoen commented 6 years ago

Windows, Visual Studio 2013

jief666 commented 6 years ago

I prefer things not to silently fail. Allowing an index out of bounds, at least in debug mode, is annoying. At least VC crash !

jief666 commented 6 years ago

HI @LubosD and @bugaevc.

Should we do a pull request for this, so it's quicker for you ? I'm trained with pull requests, so I can do that :-)

bugaevc commented 6 years ago

Yes please :)

jief666 commented 6 years ago

Which fix do you prefer ? In void initFromBuffer() like tomkoen proposed, or in HFSBTreeNode& operator=(const HFSBTreeNode& that)

jief666 commented 6 years ago

The one in initBuffer() is better, don't you think ?

jief666 commented 6 years ago

Done : #70