mirror/openmw
1
0
Fork 0
mirror of https://github.com/OpenMW/openmw.git synced 2025-02-28 17:09:41 +00:00
Code Issues Releases Wiki Activity

Fix use after move in InfoOrder::insertInfo

Browse source 
When it->second.mPosition->mPrev != value.mPrev value is first moved into
*mPosition and then used to get mPrev. Since mPrev is RefId and it's copy-only
type there is no real problem but coverity complains about it.

Also enforce contract of insertInfo to support move-only types by adding a test
for a value type with deleted copy constructors.
This commit is contained in:
elsid 2023-07-30 13:17:19 +02:00
parent d2f16774d9
commit 66c5944ecf
No known key found for this signature in database
GPG key ID: 4DE04C198CBA7625
3 changed files with 45 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
apps/openmw_test_suite/CMakeLists.txt
View file

@ -89,6 +89,7 @@ file(GLOB UNITTEST_SRC_FILES
esm3/readerscache.cpp
esm3/testsaveload.cpp
esm3/testesmwriter.cpp
esm3/testinfoorder.cpp
nifosg/testnifloader.cpp
)

27
apps/openmw_test_suite/esm3/testinfoorder.cpp Normal file
View file

@ -0,0 +1,27 @@
#include <components/esm3/infoorder.hpp>
#include <gtest/gtest.h>
namespace ESM
{
namespace
{
struct Value
{
RefId mId;
RefId mPrev;
Value() = default;
Value(const Value&) = delete;
Value(Value&&) = default;
Value& operator=(const Value&) = delete;
Value& operator=(Value&&) = default;
};
TEST(Esm3InfoOrderTest, insertInfoShouldNotCopyValue)
{
InfoOrder<Value> order;
order.insertInfo(Value{}, false);
}
}
}

29
components/esm3/infoorder.hpp
View file

@ -24,19 +24,12 @@ namespace ESM
auto it = mInfoPositions.find(value.mId);
if (it != mInfoPositions.end())
if (it != mInfoPositions.end() && it->second.mPosition->mPrev == value.mPrev)
{
bool samePrev = it->second.mPosition->mPrev == value.mPrev;
*it->second.mPosition = std::forward<V>(value);
it->second.mDeleted = deleted;
if (samePrev)
return;
return;
}
else
it = mInfoPositions.emplace(value.mId, Item{ .mPosition = mOrderedInfo.end(), .mDeleted = deleted })
.first;
Item& item = it->second;
auto before = mOrderedInfo.begin();
if (!value.mPrev.empty())
@ -47,10 +40,22 @@ namespace ESM
else
before = mOrderedInfo.end();
}
if (item.mPosition == mOrderedInfo.end())
item.mPosition = mOrderedInfo.insert(before, std::forward<V>(value));
if (it == mInfoPositions.end())
{
const RefId id = value.mId;
mInfoPositions.emplace(id,
Item{
.mPosition = mOrderedInfo.insert(before, std::forward<V>(value)),
.mDeleted = deleted,
});
}
else
mOrderedInfo.splice(before, mOrderedInfo, item.mPosition);
{
*it->second.mPosition = std::forward<V>(value);
it->second.mDeleted = deleted;
mOrderedInfo.splice(before, mOrderedInfo, it->second.mPosition);
}
}
void removeInfo(const RefId& infoRefId)