argotorg / argotorg/solidity

Undefined behaviour: Copying struct/class with uninitialized members

Open
#15,462 0 comments 0 reactions 0 assignees View on GitHub
bug :bug:
Dominant language
C++
Stars
25.7k
Forks
6.2k
Avg merge
2d 19h
Merged PRs (30d)
29

Description

Copying struct/class with uninitialized members is consider as undefined behaviour. (Some References: [[1]](https://stackoverflow.com/questions/60112841/copying-structs-with-uninitialized-members/), [[2]](https://stackoverflow.com/questions/15231527/is-it-ok-to-copy-uninitialized-data-if-it-will-be-unused-set-later)).
And I found this kind of UB in some files.

# struct Literal

Function `createAsmNode(_node)` create a `Literal` object `r`, when this function `return r;`, struct `Literal`'s implicit constructor is called, and `r`'s uninitialized members `kind` is copied to `retval`.

https://github.com/ethereum/solidity/blob/f369cdd05d71f383f1ec4a62981b08720d99796d/libyul/AsmJsonImporter.cpp#L167-L169

https://github.com/ethereum/solidity/blob/f369cdd05d71f383f1ec4a62981b08720d99796d/libyul/AsmJsonImporter.cpp#L57-L68

`kind` is not unsigned narrow character type or `std::byte`, and `LiteralKind` object does not have default-initalization.

https://github.com/ethereum/solidity/blob/f369cdd05d71f383f1ec4a62981b08720d99796d/libyul/AST.h#L69

# class AssemblyItem
## move constructor

Same as struct `Literal`, `AssemblyItem`'s uninitialzed member `m_instruction` may be copied to `retval` while returning from function `appendSubroutine`, if `m_type != Operation`.

https://github.com/ethereum/solidity/blob/f369cdd05d71f383f1ec4a62981b08720d99796d/libevmasm/Assembly.h#L107
https://github.com/ethereum/solidity/blob/26d5b3f88790183f5b30dc0067db629338846428/libevmasm/Assembly.h#L74
https://github.com/ethereum/solidity/blob/f369cdd05d71f383f1ec4a62981b08720d99796d/libevmasm/AssemblyItem.h#L75-L83

`m_instruction` is not unsigned narrow character type or `std::byte`, and will not be default initalized.

https://github.com/ethereum/solidity/blob/26d5b3f88790183f5b30dc0067db629338846428/libevmasm/AssemblyItem.h#L217-L229

## copy constructor

And also found some use of copy constructor before initializing `AssemblyItem`'s member `m_instruction`.

https://github.com/ethereum/solidity/blob/26d5b3f88790183f5b30dc0067db629338846428/libsolidity/codegen/CompilerContext.cpp#L593-L599

# Summary

May be these uninitialized members will not be used later (or will be set before using them), it's better to eliminate these UBs by initializing these members with a specified value :)

Contributor guide

Open the contributing guide

Research direction

Review the cited locations in libyul/AsmJsonImporter.cpp, libyul/AST.h, libevmasm/Assembly.h, libevmasm/AssemblyItem.h, and libsolidity/codegen/CompilerContext.cpp. Trace the return and copy-construction paths for Literal and AssemblyItem, then run the project's existing C++ build and test checks. Done means the listed paths no longer copy uninitialized members and the checks pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.