Page MenuHomePhabricator

[SECP256K1] Fix a valgrind issue in multisets
ClosedPublic

Authored by Fabien on Mar 4 2020, 17:58.

Details

Summary

This diff fixes a valgrind error: `Conditional jump or move depends on
uninitialised value(s)` in the multisets module.

This occurs in the case of a multiset becoming empty. The group element
representing the multiset value becomes inifinity, branching in a case
where the field elements will not be set. Just after the fields elements
are being normalized, triggering the valgrind error. This can occur in
the remove or the combine function.

This is actually not a real world issue, because the normalized value
will not be used: the infinity is special cased during multiset
finalization and handled correctly.

The path is covered for the remove function by the associated tests,
and I added a test case for the combine function.

Test Plan
ninja check-secp256k1

valgrind ./tests 16

Ensure the issue is gone.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
secp256k1_fix_multiset_valgrind
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9729
Build 17345: Default Diff Build & Tests
Build 17344: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 4 2020, 19:12
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/secp256k1/src/modules/multiset/main_impl.h
109

You need to put a comment here, because I just spent a long ass time figuring out why this is required, even with the explanations in the diff's description - which the next reader will not have. So be nice to yourself in 6 month.

src/secp256k1/src/modules/multiset/tests_impl.h
152

space after coma

This revision now requires changes to proceed.Mar 4 2020, 19:12
Fabien planned changes to this revision.Mar 4 2020, 20:13
Fabien added inline comments.
src/secp256k1/src/modules/multiset/main_impl.h
109

Will do, I should be more greedy with comments

src/secp256k1/src/modules/multiset/tests_impl.h
152

Was to make it consistent with the other parts of the test, most have no space (but some have). I'll add a space here and will make it consistent across the whole file in another diff.

This revision is now accepted and ready to land.Mar 5 2020, 16:48
src/secp256k1/src/modules/multiset/main_impl.h
152 ↗(On Diff #16771)

That's a strange way to solve it. Why not only normalize when the point is not infinity then?

src/secp256k1/src/modules/multiset/main_impl.h
152 ↗(On Diff #16771)

I thought of that, but I found this solution slightly better for a few reasons:

  • Even if it doesn't matter here, it's constant time
  • multiset_from_gej_var doxygen comment states that the field elements should be normalized. If one day there is a check added it would have failed.
  • As far as I can tell, this is more consistent with the other parts of the code that provide initialization to the variable if required.