Skip to content

Commit

Permalink
Fix 12060: calculate sizeof(struct) (#5919)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pfultz2
pfultz2 authored Jan 29, 2024
1 parent 381360b commit 90d7f72
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 1 deletion.
65 changes: 64 additions & 1 deletion lib/valueflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,53 @@ static void setTokenValueCast(Token *parent, const ValueType &valueType, const V
}
}

template<class F>
static size_t accumulateStructMembers(const Scope* scope, F f)
{
size_t total = 0;
for (const Variable& var : scope->varlist) {

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

I started getting Segmentation fault (core dumped) on my project after this commit. Maybe you don't check for nullptr somewhere here? @pfultz2

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

It's either scope or vt or both that should be checked for nullptr

This comment has been minimized.

Sign in to view

Copy link
@pfultz2

pfultz2 Jan 29, 2024

Author Contributor

They are both checked for null already, do you have a reduced test case?

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

I don't have a reduced test case yet, but it always crashes on the same cpp file, for example here: https://github.com/SChernykh/p2pool/actions/runs/7699485772/job/20981188938 (in the end cppcheck-ubuntu -> Run cppcheck step). I can try to modify my CI scripts to get the callstack from that core dump.

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

@pfultz2 I reproduced it locally and got this output in gdb:

12/25 files checked 48% done
Checking /home/monero/p2pool/src/p2p_server.cpp ...
Checking /home/monero/p2pool/src/p2p_server.cpp: SIZE_MAX=UINT64_MAX;RAPIDJSON_ENDIAN=RAPIDJSON_LITTLEENDIAN;__SSE2__=1;HAVE_BUILTIN_CLZLL=1;HAVE_RES_QUERY=1;MINIUPNP_STATICLIB=1;RAPIDJSON_PARSE_DEFAULT_FLAGS=kParseTrailingCommasFlag;WITH_RANDOMX=1;WITH_UPNP=1;ZMQ_STATIC=1...

Program received signal SIGSEGV, Segmentation fault.
0x0000555555e777df in std::__debug::list<Variable, std::allocator<Variable> >::begin (this=<error reading variable: Cannot access memory at address 0x7fffff7feff0>)
    at /usr/include/c++/11/debug/list:231
231           begin() const _GLIBCXX_NOEXCEPT
(gdb) bt
#0  0x0000555555e777df in std::__debug::list<Variable, std::allocator<Variable> >::begin (this=<error reading variable: Cannot access memory at address 0x7fffff7feff0>)
    at /usr/include/c++/11/debug/list:231
#1  0x0000555555e1e814 in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d81b7e0,
    f=...) at lib/valueflow.cpp:1112
#2  0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#3  0x0000555555dec3cf in operator() (__closure=0x7fffff7ff140, max=0, vt2=...) at lib/valueflow.cpp:1148
#4  0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555b9904d0,
    f=...) at lib/valueflow.cpp:1118
#5  0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#6  0x0000555555dec3cf in operator() (__closure=0x7fffff7ff260, max=0, vt2=...) at lib/valueflow.cpp:1148
#7  0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d863280,
    f=...) at lib/valueflow.cpp:1118
#8  0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#9  0x0000555555dec3cf in operator() (__closure=0x7fffff7ff380, max=4, vt2=...) at lib/valueflow.cpp:1148
#10 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d81b0c0,
    f=...) at lib/valueflow.cpp:1118
#11 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#12 0x0000555555dec3cf in operator() (__closure=0x7fffff7ff4a0, max=8, vt2=...) at lib/valueflow.cpp:1148
#13 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d81b7e0,
    f=...) at lib/valueflow.cpp:1118
#14 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#15 0x0000555555dec3cf in operator() (__closure=0x7fffff7ff5c0, max=0, vt2=...) at lib/valueflow.cpp:1148
#16 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555b9904d0,
    f=...) at lib/valueflow.cpp:1118
#17 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#18 0x0000555555dec3cf in operator() (__closure=0x7fffff7ff6e0, max=0, vt2=...) at lib/valueflow.cpp:1148
#19 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d863280,
    f=...) at lib/valueflow.cpp:1118
#20 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#21 0x0000555555dec3cf in operator() (__closure=0x7fffff7ff800, max=4, vt2=...) at lib/valueflow.cpp:1148
#22 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d81b0c0,
    f=...) at lib/valueflow.cpp:1118
#23 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#24 0x0000555555dec3cf in operator() (__closure=0x7fffff7ff920, max=8, vt2=...) at lib/valueflow.cpp:1148
#25 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d81b7e0,
    f=...) at lib/valueflow.cpp:1118
#26 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#27 0x0000555555dec3cf in operator() (__closure=0x7fffff7ffa40, max=0, vt2=...) at lib/valueflow.cpp:1148
#28 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555b9904d0,
    f=...) at lib/valueflow.cpp:1118
#29 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147
#30 0x0000555555dec3cf in operator() (__closure=0x7fffff7ffb60, max=0, vt2=...) at lib/valueflow.cpp:1148
#31 0x0000555555e1e8ad in accumulateStructMembers<getAlignOf(const ValueType&, const Settings&)::<lambda(size_t, const ValueType&)> >(const Scope *, struct {...}) (scope=0x55555d863280,
    f=...) at lib/valueflow.cpp:1118
#32 0x0000555555dec4a1 in getAlignOf (vt=..., settings=...) at lib/valueflow.cpp:1147

It crashed exactly at the line where I started this converstaion: valueflow.cpp:1112 :)

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

error reading variable: Cannot access memory at address 0x7fffff7feff0

So it wasn't a null pointer, but it was probably some use-after-free bug.

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 29, 2024

Contributor

I couldn't get the full callstack, it just kept going (more than 1000 frames): https://paste.debian.net/hidden/fde3a6e1/

This comment has been minimized.

Sign in to view

Copy link
@chrchr-github

chrchr-github Jan 31, 2024

Collaborator

See #5926

This comment has been minimized.

Sign in to view

Copy link
@SChernykh

SChernykh Jan 31, 2024

Contributor

This fixed the crash for me.
if (var.isStatic())
continue;
if (const ValueType* vt = var.valueType()) {
if (vt->type == ValueType::Type::RECORD && vt->typeScope == scope)
return 0;
total = f(total, *vt);
}
if (total == 0)
return 0;
}
return total;
}

static size_t bitCeil(size_t x)
{
if (x <= 1)
return 1;
--x;
x |= x >> 1;
x |= x >> 2;
x |= x >> 4;
x |= x >> 8;
x |= x >> 16;
x |= x >> 32;
return x + 1;
}

static size_t getAlignOf(const ValueType& vt, const Settings& settings)
{
if (vt.pointer || vt.isPrimitive()) {
auto align = ValueFlow::getSizeOf(vt, settings);
return align == 0 ? 0 : bitCeil(align);
}
if (vt.type == ValueType::Type::RECORD && vt.typeScope) {
return accumulateStructMembers(vt.typeScope, [&](size_t max, const ValueType& vt2) {
size_t a = getAlignOf(vt2, settings);
return std::max(max, a);
});
}
return 0;
}

static nonneg int getSizeOfType(const Token *typeTok, const Settings &settings)
{
const ValueType &valueType = ValueType::parseDecl(typeTok, settings);
Expand Down Expand Up @@ -1134,7 +1181,23 @@ size_t ValueFlow::getSizeOf(const ValueType &vt, const Settings &settings)
return settings.platform.sizeof_double;
if (vt.type == ValueType::Type::LONGDOUBLE)
return settings.platform.sizeof_long_double;

if (vt.type == ValueType::Type::RECORD && vt.typeScope) {
size_t total = accumulateStructMembers(vt.typeScope, [&](size_t total, const ValueType& vt2) -> size_t {
size_t n = ValueFlow::getSizeOf(vt2, settings);
size_t a = getAlignOf(vt2, settings);
if (n == 0 || a == 0)
return 0;
size_t padding = (a - (total % a)) % a;
return total + padding + n;
});
if (total == 0)
return 0;
size_t align = getAlignOf(vt, settings);
if (align == 0)
return 0;
total += (align - (total % align)) % align;
return total;
}
return 0;
}

Expand Down
57 changes: 57 additions & 0 deletions test/testvalueflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,63 @@ class TestValueFlow : public TestFixture {
values = tokenValues(code, "=");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(sizeof(std::int32_t) * 10 * 20, values.back().intvalue);

code = "struct X { float a; float b; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(8, values.back().intvalue);

code = "struct X { char a; char b; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(2, values.back().intvalue);

code = "struct X { char a; float b; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(8, values.back().intvalue);

code = "struct X { char a; char b; float c; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(8, values.back().intvalue);

code = "struct X { float a; char b; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(8, values.back().intvalue);

code = "struct X { float a; char b; char c; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(8, values.back().intvalue);

code = "struct A { float a; char b; };\n"
"struct X { A a; char b; };\n"
"void f() {\n"
" x = sizeof(X);\n"
"}";
values = tokenValues(code, "( X )");
ASSERT_EQUALS(1U, values.size());
ASSERT_EQUALS(12, values.back().intvalue);
}

void valueFlowComma()
Expand Down

0 comments on commit 90d7f72

Please sign in to comment.