cnr-isti-vclab / corto

Mesh compression library, designed for rendering and speed.
Other
196 stars 43 forks source link

Heap corruption errors while reading certain .ply files #36

Closed robertc-git closed 2 years ago

robertc-git commented 2 years ago

I was getting heap corruption errors when reading certain .ply files and found the cause: skip_property_binary() in tinyply.cpp declares the "skip" vector as static in line 112, so if it's called the first time with a small-sized property, a subsequent call on a larger-sized property will try to store data out of bounds.

Here's an example .ply header that demonstrates this issue:

format binary_little_endian 1.0
element vertex 100
property float x
property float y
property float z
property uchar prop1
property int prop2
end_header

In the above example, properties "prop1" and "prop2" will be skipped. The skip_property_binary() function is called for prop1 first, so the size of the static "skip" vector becomes 1. When skip_property_binary() is called again for prop2, the static "skip" vector still has size 1, so the is.read() call will overwrite the bounds of the skip vector, because the size of prop2 is 4 bytes.

A quick fix would be to remove "static" from line 112 of tinyply.cpp. Alternatively, if you prefer to keep it static, adding the line skip.resize(PropertyTable[property.propertyType].stride); immediately after the static declaration also fixes the bug.

Here's the existing code from tinyply.cpp, for reference:

size_t PlyFile::skip_property_binary(const PlyProperty & property, std::istream & is)
{
    static std::vector<char> skip(PropertyTable[property.propertyType].stride);
    if (property.isList)
    {
        size_t listSize = 0;
        size_t dummyCount = 0;
        read_property_binary(property.listType, &listSize, dummyCount, is);
        for (size_t i = 0; i < listSize; ++i) is.read(skip.data(), PropertyTable[property.propertyType].stride);
        return listSize;
    }
    else
    {
        is.read(skip.data(), PropertyTable[property.propertyType].stride);
        return 0;
    }
}
ponchio commented 2 years ago

Hi, thanks for the report.

Removing the static would cost a lot of memory allocations, I think it's just better to use the istream "ignore" function, so no need for an array.