Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix edge case in expand_root_of_unity #375

Merged
merged 1 commit into from Oct 10, 2023

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Oct 5, 2023

Valgrind pointed out usage of an uninitialized value in our tests. Since this is part of the trusted setup initialization, I expect this would never happen, but good to fix nevertheless.

==501666== Conditional jump or move depends on uninitialised value(s)
==501666==    at 0x40A5A6: fr_is_one (in /home/devops/jtraglia/c-kzg-4844/src/test_c_kzg_4844)
==501666==    by 0x40A556: expand_root_of_unity (in /home/devops/jtraglia/c-kzg-4844/src/test_c_kzg_4844)
==501666==    by 0x40952E: test_expand_root_of_unity__fails_wrong_root_of_unity (in /home/devops/jtraglia/c-kzg-4844/src/test_c_kzg_4844)
==501666==    by 0x4031C1: tt_execute (in /home/devops/jtraglia/c-kzg-4844/src/test_c_kzg_4844)
==501666==    by 0x403AA3: main (in /home/devops/jtraglia/c-kzg-4844/src/test_c_kzg_4844)

In the following test, we provide the wrong root of unity, which causes the loop in expand_root_of_unity to break early. Here, the width is 256 but the look will break at 128 (because it encountered a value of one).

static void test_expand_root_of_unity__fails_wrong_root_of_unity(void) {
C_KZG_RET ret;
fr_t roots[257], root_of_unity;
blst_fr_from_uint64(&root_of_unity, SCALE2_ROOT_OF_UNITY[7]);
ret = expand_root_of_unity(roots, &root_of_unity, 256);
ASSERT_EQUALS(ret, C_KZG_BADARGS);
}

In expand_root_of_unity, if the loop breaks early, the final check will occur on an uninitialized value:

c-kzg-4844/src/c_kzg_4844.c

Lines 1563 to 1567 in b2e4149

for (uint64_t i = 2; !fr_is_one(&out[i - 1]); i++) {
CHECK(i <= width);
blst_fr_mul(&out[i], &out[i - 1], root);
}
CHECK(fr_is_one(&out[width]));

I also added CHECK(width >= 2) as a sanity check and changed the loop to be more "natural" IMO.

@asn-d6
Copy link
Contributor

asn-d6 commented Oct 10, 2023

Can we just add the check CHECK(i == width); without changing the loop logic?

I feel like this is a non-trivial loop logic and the less we change it the better.

@jtraglia
Copy link
Member Author

IIRC you'd have to compare i to width - 1. It's not as pretty. I think this way is safer.

@ppopth
Copy link
Member

ppopth commented Oct 10, 2023

Even though, it changes the loop logic, I think the latter is a lot easier to understand (like you can know the maximum number of iterations immediately)

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. LGTM!

@asn-d6 asn-d6 merged commit f3fffec into ethereum:main Oct 10, 2023
33 checks passed
@jtraglia jtraglia deleted the fix-edge-case-expand-root branch October 10, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants