[test] Avoid reading a potentially uninitialized variable in tx_invalid-test (transaction_tests.cpp) #9555

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));

It is set one line before use, no?

On Sat, Jan 14, 2017 at 8:45 PM, practicalswift notifications@github.com wrote:

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

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

... triggering UB.

You can view, comment on, or merge this pull request online at:

#9555 Commit Summary

• [test] Avoid undefined behaviour in tx_invalid-test (transaction_tests.cpp)

File Changes

• M src/test/transaction_tests.cpp https://github.com/bitcoin/bitcoin/pull/9555/files#diff-0 (2)

Patch Links:

• #9555.patch
• #9555.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9555, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvxdY6fMunnH-S0n2t9WF7XiZ_BQhks5rSSXkgaJpZM4LjuLa .

@MarcoFalke Not in the cases ...

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

... right? :-)

@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? :-)

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

@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? :-)

Indeed, I was reading the wrong lines of the file.

utACK 8455e367fe79ff53ea81d283f3435e74b1633c88

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?

@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

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

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