[SECP256K1] Fix a valgrind issue in multisets
ClosedPublic
Actions

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING4dd5e216df48: [SECP256K1] Fix a valgrind issue in multisets
rABC4dd5e216df48: [SECP256K1] Fix a valgrind issue in multisets

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Mar 4 2020, 17:58

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 4 2020, 17:58

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9729: Diff 16768.Mar 4 2020, 18:09

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 ↗	(On Diff #16768)	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 ↗	(On Diff #16768)	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 ↗	(On Diff #16768)	Will do, I should be more greedy with comments
src/secp256k1/src/modules/multiset/tests_impl.h
152 ↗	(On Diff #16768)	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.

Address feedback.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9731: Diff 16771.Mar 4 2020, 20:51

Fabien added a child revision: D5433: [SECP256K1] Add valgrind check to travis.Mar 5 2020, 09:39

deadalnix accepted this revision.Mar 5 2020, 16:48

This revision is now accepted and ready to land.Mar 5 2020, 16:48

deadalnix added inline comments.Mar 5 2020, 16:50

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?

Fabien added inline comments.Mar 5 2020, 17:09

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.