Skip to content

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 14, 2017

Prior to this commit the err variable was not guaranteed to be set before the check ...

BOOST_CHECK_MESSAGE(err != SCRIPT_ERR_OK, ScriptErrorString(err));

@practicalswift practicalswift force-pushed the avoid-ub-in-tx_invalid-test branch from 0ed1087 to dbb285a Compare January 14, 2017 19:53
@maflcko
Copy link
Member

maflcko commented Jan 14, 2017 via email

@practicalswift
Copy link
Contributor Author

@MarcoFalke Not in the cases ...

  • tx.vin.size() == 0, or
  • !(CheckTransaction(tx, state) && state.IsValid())

... right? :-)

@fanquake fanquake added the Tests label Jan 15, 2017
@practicalswift
Copy link
Contributor Author

@MarcoFalke After re-reading your comment I think that you might be referring to err in the context of lines 172-175 whereas my PR is referring to err on lines 242-260. Is my interpretation correct? :-)

@practicalswift practicalswift changed the title [test] Avoid triggering undefined behaviour in tx_invalid-test (transaction_tests.cpp) [test] Avoid potentially reading an uninitialized variable in tx_invalid-test (transaction_tests.cpp) Jan 15, 2017
@practicalswift practicalswift force-pushed the avoid-ub-in-tx_invalid-test branch from dbb285a to 6087290 Compare January 15, 2017 21:59
@practicalswift practicalswift changed the title [test] Avoid potentially reading an uninitialized variable in tx_invalid-test (transaction_tests.cpp) [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (transaction_tests.cpp) Jan 15, 2017
@practicalswift practicalswift force-pushed the avoid-ub-in-tx_invalid-test branch 2 times, most recently from e4c8db6 to 30e6c25 Compare January 15, 2017 22:00
@luke-jr
Copy link
Member

luke-jr commented Jan 20, 2017

It may be better to leave this be, so memory sanitizers and static analysis can detect cases where we fail to set it?

@practicalswift practicalswift force-pushed the avoid-ub-in-tx_invalid-test branch from 30e6c25 to 604228c Compare January 21, 2017 09:50
@practicalswift
Copy link
Contributor Author

@luke-jr Good point about catching cases where we fail to set it. My suggestion is that we initialize to SCRIPT_ERR_OK. That will make sure that failures to set err are always detected since the test asserts that err != SCRIPT_ERR_OK. See updated version. Looks OK? :-)

…id-test

Prior to this commit the err variable was not guaranteed to be set before
the check ...

    BOOST_CHECK_MESSAGE(err != SCRIPT_ERR_OK, ScriptErrorString(err));
@practicalswift practicalswift force-pushed the avoid-ub-in-tx_invalid-test branch from 604228c to 8455e36 Compare January 21, 2017 09:57
@maflcko
Copy link
Member

maflcko commented Jan 22, 2017 via email

@luke-jr
Copy link
Member

luke-jr commented Feb 2, 2017

That will still suppress compiler and analyser warnings of using unassigned values. Is there a way we can initialise it, yet tell the compiler/etc to treat the value as uninitialised still?

@practicalswift
Copy link
Contributor Author

@luke-jr I don't know if that is possible and/or how to do it, but if possible I'll add such an annotation :-) Let me know if you find any info on how to achieve that

@maflcko
Copy link
Member

maflcko commented Mar 6, 2017

As long as the tests fail when the variable is not assigned the correct value in the code, all is fine.

@practicalswift
Copy link
Contributor Author

@MarcoFalke Yes, that should be the case now :-)

@maflcko maflcko merged commit 8455e36 into bitcoin:master Mar 7, 2017
maflcko pushed a commit that referenced this pull request Mar 7, 2017
…e in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 21, 2019
…variable in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 27, 2019
…variable in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2019
…variable in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 5, 2019
…variable in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 5, 2019
…variable in tx_invalid-test (transaction_tests.cpp)

8455e36 [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (practicalswift)

Tree-SHA512: 1064cdd5c9e4612a05397a5880535d93dbb18dec4897b4bbda9e6ad78d30f4c72303e4d23159398f1b33545ff5819e739e374d7cde757e402b26c355268a2319
@practicalswift practicalswift deleted the avoid-ub-in-tx_invalid-test branch April 10, 2021 19:29
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants