Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) #5707

dirkmueller · 2023-11-27T23:03:45Z

As reported in https://sourceforge.net/p/cppcheck/discussion/general/thread/fa43fb8ab1/ removeContradiction() minValue/maxValue.remove(..) can access free'd memory as it removes all matching values by iterating over the complete list. Creating a full copy instead of a reference avoids this issue.

firewave · 2023-11-27T23:25:07Z

Thanks for your contribution.

Please add the POC code to the unit tests. Based on the code this should most likely go into testgarbage.cpp.

lib/token.cpp

dirkmueller · 2023-11-28T07:01:23Z

I wonder if the better fix would be to use erase() instead of remove() as I don't actually see the reason for iterating over the whole list.

dirkmueller · 2023-11-28T07:09:12Z

dirkmueller@67f3f77 is doing that. seems slightly faster also.

firewave · 2023-11-28T13:15:42Z

I cannot reproduce the ASAN failure locally with either the unit test or via an external file. I tried GCC and Clang.

dirkmueller · 2023-11-28T15:08:18Z

I cannot reproduce the ASAN failure locally with either the unit test or via an external file. I tried GCC and Clang.

yes, it is very tricky to trigger. Like the original reporter, with the given reproducer it only triggers on GCC 7.x. the issue is only happening when the remove() finds more than one element, and the first element remove triggers a resize and reallocate down. when it doesn't happen (as is the case with any other implementation) there is no ASAN violation logged.

That's why I originally didn't add the testcase to the PR because it's not gonna trigger reliable. Still I think the deep copy avoids the issue as does a conversion of the code from remove() to erase() does.

dirkmueller · 2023-11-28T22:59:59Z

(an easy way to trigger is by running it inside an opensuse leap 15.4 container - can provide instructions if wanted)

firewave · 2023-11-29T20:35:24Z

GCC 7 is available on ubuntu 20.04 which is one of my local systems I will give it a spin tomorrow.

firewave · 2023-11-30T10:35:31Z

I can reproduce the issue with GCC 7 and valgrind:

==1068== Invalid read of size 4
==1068==    at 0x913125: ValueFlow::Value::equalValue(ValueFlow::Value const&) const (vfvalue.h:61)
==1068==    by 0x9132EE: ValueFlow::Value::operator==(ValueFlow::Value const&) const (vfvalue.h:150)
==1068==    by 0xE85203: std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::remove(ValueFlow::Value const&) (list:649)
==1068==    by 0xE74545: removeContradiction(std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >&) (token.cpp:2008)
==1068==    by 0xE753AD: removeContradictions(std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >&) (token.cpp:2142)
==1068==    by 0xE75B63: Token::addValue(ValueFlow::Value const&) (token.cpp:2232)
==1068==    by 0x8BF74B: setTokenValue(Token*, ValueFlow::Value, Settings const*, SourceLocation) (valueflow.cpp:623)
==1068==    by 0x8C0E37: setTokenValue(Token*, ValueFlow::Value, Settings const*, SourceLocation) (valueflow.cpp:774)
==1068==    by 0x8CA4BD: valueFlowImpossibleValues(TokenList&, Settings const*) (valueflow.cpp:1842)
==1068==    by 0x8F1126: ValueFlow::setValues(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, TimerResultsIntf*)::{lambda(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, std::__debug::set<Scope const*, std::less<Scope const*>, std::allocator<Scope const*> > const&)#15}::operator()(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, std::__debug::set<Scope const*, std::less<Scope const*>, std::allocator<Scope const*> > const&) const (valueflow.cpp:9467)
==1068==    by 0x9121A0: ValueFlowPassAdaptor<ValueFlow::setValues(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, TimerResultsIntf*)::{lambda(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, std::__debug::set<Scope const*, std::less<Scope const*>, std::allocator<Scope const*> > const&)#15}>::run(ValueFlowState const&) const (valueflow.cpp:9395)
==1068==    by 0x92CDC2: ValueFlowPassRunner::run(ValuePtr<ValueFlowPass> const&) const (valueflow.cpp:9323)
==1068==  Address 0x534cb80 is 16 bytes inside a block of size 272 free'd
==1068==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1068==    by 0x9A278D: __gnu_cxx::new_allocator<std::__cxx1998::_List_node<ValueFlow::Value> >::deallocate(std::__cxx1998::_List_node<ValueFlow::Value>*, unsigned long) (new_allocator.h:125)
==1068==    by 0x98977D: std::allocator_traits<std::allocator<std::__cxx1998::_List_node<ValueFlow::Value> > >::deallocate(std::allocator<std::__cxx1998::_List_node<ValueFlow::Value> >&, std::__cxx1998::_List_node<ValueFlow::Value>*, unsigned long) (alloc_traits.h:462)
==1068==    by 0x96B091: std::__cxx1998::__cxx11::_List_base<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_put_node(std::__cxx1998::_List_node<ValueFlow::Value>*) (stl_list.h:387)
==1068==    by 0x980BA0: std::__cxx1998::__cxx11::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_erase(std::__cxx1998::_List_iterator<ValueFlow::Value>) (stl_list.h:1820)
==1068==    by 0x96B01C: std::__cxx1998::__cxx11::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::erase(std::__cxx1998::_List_const_iterator<ValueFlow::Value>) (list.tcc:157)
==1068==    by 0x94932A: std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_erase(std::__cxx1998::_List_const_iterator<ValueFlow::Value>) (list:491)
==1068==    by 0xE8522D: std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::remove(ValueFlow::Value const&) (list:650)
==1068==    by 0xE74545: removeContradiction(std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >&) (token.cpp:2008)
==1068==    by 0xE753AD: removeContradictions(std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >&) (token.cpp:2142)
==1068==    by 0xE75B63: Token::addValue(ValueFlow::Value const&) (token.cpp:2232)
==1068==    by 0x8BF74B: setTokenValue(Token*, ValueFlow::Value, Settings const*, SourceLocation) (valueflow.cpp:623)
==1068==  Block was alloc'd at
==1068==    at 0x483BE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1068==    by 0x9B5493: __gnu_cxx::new_allocator<std::__cxx1998::_List_node<ValueFlow::Value> >::allocate(unsigned long, void const*) (new_allocator.h:111)
==1068==    by 0x9A5183: std::allocator_traits<std::allocator<std::__cxx1998::_List_node<ValueFlow::Value> > >::allocate(std::allocator<std::__cxx1998::_List_node<ValueFlow::Value> >&, unsigned long) (alloc_traits.h:436)
==1068==    by 0x98E396: std::__cxx1998::__cxx11::_List_base<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_get_node() (stl_list.h:383)
==1068==    by 0x97DCA3: std::__cxx1998::_List_node<ValueFlow::Value>* std::__cxx1998::__cxx11::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_create_node<ValueFlow::Value>(ValueFlow::Value&&) (stl_list.h:572)
==1068==    by 0x95E72F: void std::__cxx1998::__cxx11::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::_M_insert<ValueFlow::Value>(std::__cxx1998::_List_iterator<ValueFlow::Value>, ValueFlow::Value&&) (stl_list.h:1801)
==1068==    by 0x940766: std::__cxx1998::__cxx11::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::push_back(ValueFlow::Value&&) (stl_list.h:1123)
==1068==    by 0xE75A82: Token::addValue(ValueFlow::Value const&) (token.cpp:2222)
==1068==    by 0x8BF74B: setTokenValue(Token*, ValueFlow::Value, Settings const*, SourceLocation) (valueflow.cpp:623)
==1068==    by 0x8C0E37: setTokenValue(Token*, ValueFlow::Value, Settings const*, SourceLocation) (valueflow.cpp:774)
==1068==    by 0x8C90D4: valueFlowBitAnd(TokenList&, Settings const*) (valueflow.cpp:1638)
==1068==    by 0x8F107E: ValueFlow::setValues(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, TimerResultsIntf*)::{lambda(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, std::__debug::set<Scope const*, std::less<Scope const*>, std::allocator<Scope const*> > const&)#12}::operator()(TokenList&, SymbolDatabase&, ErrorLogger*, Settings const*, std::__debug::set<Scope const*, std::less<Scope const*>, std::allocator<Scope const*> > const&) const (valueflow.cpp:9461)

danmar · 2023-12-01T14:02:04Z

I am not strongly against the unit test..

however it's not by design to handle garbage code in valueflow etc.. at some random point in time in the future cppcheck might reject this code and say there is syntax error... and then this test will not test if there is a regression in the valueflow anymore..

if we really wanted to unit test that there is not this bug in removeContradiction() I guess we could manually call removeContradiction() in the test with certain input data however I do feel that is unfortunate since the function has internal linkage.

danmar · 2023-12-01T14:06:07Z

if we really wanted to unit test that there is not this bug in removeContradiction() I guess we could manually call removeContradiction() in the test with certain input data however I do feel that is unfortunate since the function has internal linkage.

would it make sense to create a unit test that creates a token "&" or whatever it is.. and then call the Token::addValue with whatever is problematic?

dirkmueller · 2023-12-11T21:33:13Z

I am not strongly against the unit test..

I'm actually not in favor of a unit test for this, but it was requested in the review.

if we really wanted to unit test that there is not this bug in removeContradiction() I guess we could manually call removeContradiction() in the test with certain input data however I do feel that is unfortunate since the function has internal linkage.

Right. I think in the current state the unit test is fair. it won't help but it will also not harm as there is a comment that the intention is "don't crash". I expect it to get deleted whenever cppcheck starts raising an error on the invalid code.

If we're nervous that somebody will revert the patch accidentally and reintroduce the issue, then the only path forward is to rewrite the code to not use .remove() anymore but only .erase() .

firewave · 2023-12-12T16:01:48Z

I'm actually not in favor of a unit test for this, but it was requested in the review.

Any fix should have an accompanied test case. That was not the case several years ago which is why there have been several regressions across the code.

If there is no test case to trigger this then there should be no need for the change at all. Even if there is currently only one known configuration that causes this there's still a infinite amount of unknown ones which might also encounter this.

Also the underlying code for handling this might change in the future and that might introduce a different issue which would be uncovered by the test.

danmar · 2023-12-14T17:56:33Z

@dirkmueller

I think the fix itself sounds good. Thanks for your explanation that makes sense to me.

I don't think it's a good idea to test this with garbage code.. it's not guaranteed that removeContradiction() will be called when the input is garbage.

how can I reproduce this with a token? Something like:

Token tok("&");
tok.addValue(1);
tok.addValue(2);

Can you give me feedback ..? I can add the unit test if you want.

danmar · 2023-12-16T14:17:04Z

@dirkmueller
I still think the explanation is strange. The maxValue is not used after the remove below.

One thing that looks suspicious to me in removeContradiction is:

            if (removex && removePointValue(values, x))
                bail = true;
            if (removey && removePointValue(values, y))
                bail = true;

if removePointValue removes "x" from values then "x" will obviously be invalid. But the loop continues and "x" is used again..

I am not sure how a range-for-loop behaves if elements in the container are removed during the looping. Is it defined that it's safe at all.. and is it defined that in the next loop the next element is accessed or will 1 item be skipped?

I have implemented a possible test case after logging what removeContradiction calls for the garbage code. Please feel free to try out if it can reproduce the crash for you. Add it in testtoken.cpp:

    void addValueRemoveContradictionCrash() const {
        Token tok;
        tok.str(":");
        tok.addValue(ValueFlow::Value(0));
        tok.addValue(ValueFlow::Value(8));

        // !<=-1
        ValueFlow::Value impossibleValue(-1);
        impossibleValue.valueKind = ValueFlow::Value::ValueKind::Impossible;
        impossibleValue.bound = ValueFlow::Value::Bound::Upper;
        tok.addValue(impossibleValue); // Do not crash

        // !>=2
        ValueFlow::Value impossibleValue2(2);
        impossibleValue2.valueKind = ValueFlow::Value::ValueKind::Impossible;
        impossibleValue2.bound = ValueFlow::Value::Bound::Lower;
        tok.addValue(impossibleValue2); // Do not crash
    }

danmar · 2023-12-16T14:18:56Z

I expect it to get deleted whenever cppcheck starts raising an error on the invalid code.

I think we will always keep the test code, but change the assertion so it checks that a syntax error is reported. So it will no longer test that we don't have regressions in the removeContradiction. This is why I am against a garbage code test.

test/testtoken.cpp

danmar · 2023-12-17T13:43:23Z

I would be interested to reproduce this. Can I reproduce it in a docker container?

I tried this:

$ docker run -v $HOME/cppcheck:/cppcheck -it ubuntu:20.04 /bin/bash
apt-get update
apt-get upgrade
apt-get install g++-7 valgrind make
cd /cppcheck
make clean
make -j12 CXX=g++-7
valgrind ./cppcheck garbage1.c

output from valgrind:

==1915== Memcheck, a memory error detector
==1915== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1915== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==1915== Command: ./cppcheck garbage1.c
==1915== 
Checking garbage1.c ...
Active checkers: 40/592
==1915== 
==1915== HEAP SUMMARY:
==1915==     in use at exit: 0 bytes in 0 blocks
==1915==   total heap usage: 23,814 allocs, 23,814 frees, 4,208,446 bytes allocated
==1915== 
==1915== All heap blocks were freed -- no leaks are possible
==1915== 
==1915== For lists of detected and suppressed errors, rerun with: -s
==1915== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

dirkmueller · 2023-12-17T18:24:58Z

@dirkmueller I still think the explanation is strange. The maxValue is not used after the remove below.

The crash is within the same single remove() invocation.

            if (removex && removePointValue(values, x))
                bail = true;
            if (removey && removePointValue(values, y))
                bail = true;
if removePointValue removes "x" from values then "x" will obviously be invalid. But the loop continues and "x" is used again..

it sets bail to true which returns true immediately. Unless I'm missing what you're referring to.

I am not sure how a range-for-loop behaves if elements in the container are removed during the looping. Is it defined that it's safe at all.. and is it defined that in the next loop the next element is accessed or will 1 item be skipped?

It is generally unsafe to continue in a range-for-loop if an element of the iterated container is removed. That's where erase(), which advances the iterator, is used for. I made a variant with that under #5707 (comment)

I have implemented a possible test case after logging what removeContradiction calls for the garbage code. Please feel free to try out if it can reproduce the crash for you. Add it in testtoken.cpp:

Thank you for helping with the test case. actually, while retesting, I noticed that no testcase is needed at all, as the existing testcases trigger this issue in multiple places. I'll push an updated patch shortly after finishing testing.

danmar · 2023-12-17T18:40:53Z

I noticed that no testcase is needed at all, as the existing testcases trigger this issue in multiple places

good 👍

dirkmueller · 2023-12-17T19:29:48Z

I would be interested to reproduce this. Can I reproduce it in a docker container?

yes, you can.


docker/podman run -v $HOME/cppcheck:/cppcheck  -it registry.opensuse.org/opensuse/leap:15.5 /bin/bash
cd cppcheck
mkdir build-leap
cd build-leap
zypper in cmake gcc-c++
cmake .. -DANALYZE_ADDRESS=ON -DANALYZE_MEMORY=OFF -DBUILD_TESTS=ON
make -j16
ctest -j8 --stop-on-failure
ctest --rerun-failed --output-on-failure

...

==2619==ERROR: AddressSanitizer: heap-use-after-free on address 0x612000302d50 at pc 0x0000019262aa bp 0x7fffbf9f72b0 sp 0x7fffbf9f72a8
READ of size 4 at 0x612000302d50 thread T0
    #0 0x19262a9 in ValueFlow::Value::equalValue(ValueFlow::Value const&) const /cppcheck/lib/vfvalue.h:61
    #1 0x19266fa in ValueFlow::Value::operator==(ValueFlow::Value const&) const (/cppcheck/build-leap/bin/testrunner+0x19266fa)
    #2 0x2429de1 in std::__debug::list<ValueFlow::Value, std::allocator<ValueFlow::Value> >::remove(ValueFlow::Value const&) /usr/include/c++/7/debug/list:649
    #3 0x2401b28 in removeContradiction /cppcheck/lib/token.cpp:2017
    #4 0x24043a6 in removeContradictions /cppcheck/lib/token.cpp:2142
    #5 0x240588b in Token::addValue(ValueFlow::Value const&) /cppcheck/lib/token.cpp:2232
    #6 0x1851cb1 in setTokenValue /cppcheck/lib/valueflow.cpp:623
    #7 0x19383b5 in ValueFlowAnalyzer::update(Token*, Analyzer::Action, Analyzer::Direction) (/cppcheck/build-leap/bin/testrunner+0x19383b5)
    #8 0x215809b in update /cppcheck/lib/forwardanalyzer.cpp:230
    #9 0x2158240 in operator() /cppcheck/lib/forwardanalyzer.cpp:244
    #10 0x21619b6 in traverseTok<Token, (anonymous namespace)::ForwardTraversal::updateTok(Token*, Token**)::<lambda(Token*)> > /cppcheck/lib/forwardanalyzer.cpp:163
    #11 0x2158346 in updateTok /cppcheck/lib/forwardanalyzer.cpp:245
    #12 0x216011f in updateRange /cppcheck/lib/forwardanalyzer.cpp:809
    #13 0x2160923 in valueFlowGenericForward(Token*, Token const*, ValuePtr<Analyzer> const&, Settings const&) /cppcheck/lib/forwardanalyzer.cpp:888
    #14 0x1872765 in valueFlowForward /cppcheck/lib/valueflow.cpp:2058
    #15 0x1872c6b in valueFlowForward /cppcheck/lib/valueflow.cpp:2071

danmar · 2023-12-17T20:26:39Z

yes, you can.

thanks, I copy pasted your commands and it reproduce the error.

…move) As reported in https://sourceforge.net/p/cppcheck/discussion/general/thread/fa43fb8ab1/ removeContradiction() minValue/maxValue.remove(..) can access free'd memory as it removes all matching values by iterating over the complete list. Switch to use .erase() and using iterators to avoid that. Signed-off-by: Dirk Müller <[email protected]>

danmar · 2023-12-19T15:01:57Z

it's really sneaky.. to my bare eyes the preprocessor code looks safe.

danmar · 2023-12-19T19:42:59Z

Thanks! So somehow it would be good to start using opensuse in our testing also so we can detect regressions.

orbitcowboy reviewed Nov 28, 2023

View reviewed changes

lib/token.cpp Outdated Show resolved Hide resolved

dirkmueller force-pushed the use_after_free_remove branch from bc63dbc to 770d4b6 Compare November 28, 2023 06:59

dirkmueller requested a review from danmar December 11, 2023 21:34

dirkmueller force-pushed the use_after_free_remove branch from 770d4b6 to 944e838 Compare December 17, 2023 10:04

danmar reviewed Dec 17, 2023

View reviewed changes

test/testtoken.cpp Outdated Show resolved Hide resolved

danmar changed the title ~~removeContradiction() Avoid use-after-free on multiple remove~~ Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) Dec 17, 2023

dirkmueller force-pushed the use_after_free_remove branch from 944e838 to 8437a2f Compare December 17, 2023 19:44

dirkmueller force-pushed the use_after_free_remove branch from 8437a2f to f161fa7 Compare December 17, 2023 21:00

dirkmueller requested a review from danmar December 19, 2023 08:56

danmar merged commit 76695f6 into danmar:main Dec 19, 2023
68 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) #5707

Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) #5707

dirkmueller commented Nov 27, 2023

firewave commented Nov 27, 2023

dirkmueller commented Nov 28, 2023

dirkmueller commented Nov 28, 2023

firewave commented Nov 28, 2023

dirkmueller commented Nov 28, 2023 •

edited

Loading

dirkmueller commented Nov 28, 2023

firewave commented Nov 29, 2023

firewave commented Nov 30, 2023

danmar commented Dec 1, 2023 •

edited

Loading

danmar commented Dec 1, 2023 •

edited

Loading

dirkmueller commented Dec 11, 2023 •

edited

Loading

firewave commented Dec 12, 2023

danmar commented Dec 14, 2023 •

edited

Loading

danmar commented Dec 16, 2023 •

edited

Loading

danmar commented Dec 16, 2023 •

edited

Loading

danmar commented Dec 17, 2023

dirkmueller commented Dec 17, 2023

danmar commented Dec 17, 2023

dirkmueller commented Dec 17, 2023 •

edited

Loading

danmar commented Dec 17, 2023

danmar commented Dec 19, 2023

danmar commented Dec 19, 2023

Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) #5707

Fix #12272 (removeContradiction() Avoid use-after-free on multiple remove) #5707

Conversation

dirkmueller commented Nov 27, 2023

firewave commented Nov 27, 2023

dirkmueller commented Nov 28, 2023

dirkmueller commented Nov 28, 2023

firewave commented Nov 28, 2023

dirkmueller commented Nov 28, 2023 • edited Loading

dirkmueller commented Nov 28, 2023

firewave commented Nov 29, 2023

firewave commented Nov 30, 2023

danmar commented Dec 1, 2023 • edited Loading

danmar commented Dec 1, 2023 • edited Loading

dirkmueller commented Dec 11, 2023 • edited Loading

firewave commented Dec 12, 2023

danmar commented Dec 14, 2023 • edited Loading

danmar commented Dec 16, 2023 • edited Loading

danmar commented Dec 16, 2023 • edited Loading

danmar commented Dec 17, 2023

dirkmueller commented Dec 17, 2023

danmar commented Dec 17, 2023

dirkmueller commented Dec 17, 2023 • edited Loading

danmar commented Dec 17, 2023

danmar commented Dec 19, 2023

danmar commented Dec 19, 2023

dirkmueller commented Nov 28, 2023 •

edited

Loading

danmar commented Dec 1, 2023 •

edited

Loading

danmar commented Dec 1, 2023 •

edited

Loading

dirkmueller commented Dec 11, 2023 •

edited

Loading

danmar commented Dec 14, 2023 •

edited

Loading

danmar commented Dec 16, 2023 •

edited

Loading

danmar commented Dec 16, 2023 •

edited

Loading

dirkmueller commented Dec 17, 2023 •

edited

Loading