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

Verify that blst_uint64_from_fr() works with uninit input #204

Closed
asn-d6 opened this issue Mar 13, 2023 · 2 comments
Closed

Verify that blst_uint64_from_fr() works with uninit input #204

asn-d6 opened this issue Mar 13, 2023 · 2 comments

Comments

@asn-d6
Copy link
Contributor

asn-d6 commented Mar 13, 2023

This is a discussion from #196:

Just out of curiosity, what happened in f4c2749 ?

That was a finding from -fsantitize=memory before it crashed with this error:

$ make sanitize_memory
Running memory sanitizer
FATAL: Code 0xaaab4cc9c1e0 is out of application range. Non-PIE build?
FATAL: MemorySanitizer can not mmap the shadow memory.
FATAL: Make sure to compile with -fPIE and to link with -pie.
FATAL: Disabling ASLR is known to cause this error.
FATAL: If running under GDB, try 'set disable-randomization off'.

I felt like because of the error, it was showing an incomplete list of findings. I plan to investigate why this is happening more later, but I suspect it's because blst isn't built with -fsanitize=memory. It seemed wrong to fix one instance when there are other instances of the same thing elsewhere, for example the original + one more:

c-kzg-4844/src/c_kzg_4844.c

Lines 228 to 232 in da83e45

static bool fr_is_one(const fr_t *p) {
uint64_t a[4];
blst_uint64_from_fr(a, p);
return a[0] == 1 && a[1] == 0 && a[2] == 0 && a[3] == 0;
}

c-kzg-4844/src/c_kzg_4844.c

Lines 243 to 248 in da83e45

static bool fr_equal(const fr_t *aa, const fr_t *bb) {
uint64_t a[4], b[4];
blst_uint64_from_fr(a, aa);
blst_uint64_from_fr(b, bb);
return a[0] == b[0] && a[1] == b[1] && a[2] == b[2] && a[3] == b[3];
}

I may make this change again later. I don't think they are crucial though.

Originally posted by @jtraglia in #196 (comment)

@asn-d6
Copy link
Contributor Author

asn-d6 commented Mar 20, 2023

Unfortunately, from_mont_256() which is called by blst_uint64_from_fr() is assembly so it's hard to comprehensively review.

However, I find it quite unlikely that a function whose purpose is to fill an input array would expect that array to be initialized. There is no reason to read from that array, except from places it has already written in. I think it's reasonable to close this ticket, but I will let @jtraglia also take a look as he raised the concern first.

@jtraglia
Copy link
Member

Yeah, I don't think this is an issue.

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

No branches or pull requests

2 participants