Yul: NoOutputAssembly assigns functions instead of appending #16127
Conversation
There was an error in that the functions were already part of the dialect (via the parent constructor) and the no output versions were appended instead of overwritten. This causes the vector to be re-allocated and therefore could invalidate pointers to its elements.
clonker
force-pushed
the
fix_no_output_dialect_asan
branch
from
8a17030 to
fde1aaf
Compare
| @@ -212,13 +212,13 @@ NoOutputEVMDialect::NoOutputEVMDialect(EVMDialect const& _copyFrom): | |||
| // them in one go, later reference pointers to this static vector | |||
| static std::vector<BuiltinFunctionForEVM> noOutputBuiltins = defineNoOutputBuiltins(); | |||
|
|
|||
| m_functions.reserve(m_functions.size()); | |||
There was a problem hiding this comment.
Hm, so this should in theory never do anything, as reserve will only perform a reallocation if and only if capacity() is smaller than the new requested capacity. size() should thus technically always be smaller or equal to capacity(), as auto reallocation (i.e. capacity increase) is only performed when you want to insert an element that would exceed the container's capacity.
What I'm trying to say is that ASAN was likely wrong here, but this then begs another question - the old implementation appends builtin handles to m_functions, whereas the new one overwrites the current ones (which is good and correct) - but then, how come this wasn't caught in any tests? Do we even have tests for no output builtins?
There was a problem hiding this comment.
The issue was with emplace_back down below. This is indeed a no-op so I removed it as well. We are looping over m_functions and were simultaneously appending to it.
There was a problem hiding this comment.
Yeah, that part I understand - we had bugs like these before, which are usually quite insidious and can't be reproduced via tests quite well; my question was more so aimed at the semantic difference between the two (assume iterator invalidation is not an issue) - old implementation appends, where as the new one overwrites - this should in theory mean that in the old case m_functions will always have more elements than in the new (overwritten) version?
In any case, it's used in the compatibility checker, so no big deal, but still weird that it wasn't caught by some test. Although from what I can see, we have no such tests, so this makes perfect sense :)
There was a problem hiding this comment.
Yeah the m_functions is populated in the superclass and has to be same in size as the one in the NoOutputDialect that inherits from it. Appending was just wrong but not wrong enough apparently to warrant an outright crash. Semantic difference is that the the compilability checker and stack compressors would have taken the actual builtins, not the ones that are nulled out / stubbed.
|
Please remember to link to the available context in the description. I'm assuming this is fixing the CI breakage that resulted from #15961? |
There was an implementation error in that the functions were already part of the dialect (via the parent constructor) and the no output versions were appended instead of overwritten. This causes the vector to be re-allocated and therefore could invalidate pointers to its elements while looping over it.
Introduced in #15961, discovered here https://app.circleci.com/pipelines/github/ethereum/solidity/40009/workflows/9d961d8e-2275-438b-82ee-311147bdba51