Don't use reserved identifiers memczero and benchmark_verify_t #835

real-or-random · 2020-10-20T13:01:53Z

As identified in #829 and #833. Fixes #829.

Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.

This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833.

As identified in bitcoin-core#829 and bitcoin-core#833. Fixes bitcoin-core#829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent.

elichai · 2020-10-20T14:08:03Z

FWIW http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2572.pdf

I wonder if we can use clang-tidy or something like that to catch these UBs in the CI

gmaxwell · 2020-10-21T07:11:46Z

@elichai see #833

real-or-random · 2020-10-26T12:43:24Z

@elichai @gmaxwell Mind to review? It's trivial.

elichai · 2020-10-26T12:56:22Z

Just realized,
are these also UB? :O

secp256k1/src/util.h

Lines 269 to 270 in ac05f61

    
           SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t; 
        
           SECP256K1_GNUC_EXT typedef __int128 int128_t;

elichai · 2020-10-26T12:58:17Z

tACK e89278f

real-or-random · 2020-10-26T13:33:14Z

Just realized,
are these also UB? :O

secp256k1/src/util.h

Lines 269 to 270 in ac05f61

SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;

SECP256K1_GNUC_EXT typedef __int128 int128_t;

Yeah, probably. We use __ types but we don't declare them, so this fine. But we define types reserved for the standard lib, so I guess this is evil, yes... And this is probably the only case where we may see a real clash in practice in the future, if some compiler defines both int128_t and __int128_t. (Even then is unlikely to cause issues because it's hopefully the same type.) What we could do is to guard these typedefs with defined(INT128_MAX). Does this make sense? I could add this to the PR.

IMO the entire libary would be more readable if we used typedefs like i32 and u128. But we'd get a huge diff now, and would interrupt all PRs, pretty much like clang-format.

sipa · 2020-10-26T16:56:56Z

@real-or-random Replacing all int types everywhere is invasive, but I think can reasonably change uint128_t into uint128.

gmaxwell · 2020-10-26T23:01:37Z

I had thought the int128 typedefs only ran if they weren't already defined. I agree clashing here is not great.

Personally I really don't like it when applications make local renames of all the generic standard types: Then I have to go digging through the code to make sure their custom type is defined to be the thing I think it is and that it's consistently the thing I think it is and isn't something different on some platform or another. Among other issues you end up with interface uglyness where your custom defined types end up in the interfaces and leak into applications. It was excusable back before there were standard fixed-size types, but isn't really today, and I think today it mostly still lingers on as copying a style that doesn't make sense anymore.

This is distinct from e.g. making a type for a particular kind of data like a scalar or a field type, because at least that is defined to be fit for a particular purpose. In most cases using a for-application-type is preferable to using generic types (and, in fact, mandated pretty much universally by MISRA C).

If a generic type needs to be renamed for some compatibility reason-- e.g. there isn't a universally available generic type with the required properties, then sure.

sipa · 2020-10-27T02:39:20Z

ACK e89278f

real-or-random · 2020-10-27T15:01:58Z

I had thought the int128 typedefs only ran if they weren't already defined.

I added a commit that implements this. I also added MAX and MIN macros

Note: Currently there's no compiler that provides (u)int128_t because that will open a can of worms due to (u)intmax_t. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2425.pdf .

src/util.h

jonasnick

Just to spell it out, the assumption is that a compiler that adds int128_t in the future also defines UINT128_MAX?

Looks good in general. Not sure how useful it is to define constants we don't use.

real-or-random · 2020-11-04T17:36:45Z

Just to spell it out, the assumption is that a compiler that adds int128_t in the future also defines UINT128_MAX?

Yes. And this is implied by the standard as far as I understand it.

Looks good in general. Not sure how useful it is to define constants we don't use.

Yeah, one of the reasons why I decided for it is that it increases the probability to have a clash be apparent at compile-time (and fail compilation).

jonasnick

ACK 1f4dd03

sipa · 2020-11-04T23:23:51Z

utACK 1f4dd03

…ify_t Summary: ``` As identified in #829 and #833. Fixes #829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent. Typedef (u)int128_t only when they're not provided by the compiler ``` Backport of [[bitcoin-core/secp256k1#835 | secp256k1#835]] Test Plan: ninja check-secp256k1 bench-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9373

…vival of #636) 765ef53 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner) 349e6ab Introduce separate _clear functions for hash module (Tim Ruffing) 99cc9fd Don't rely on memset to set signed integers to 0 (Tim Ruffing) 97c57f4 Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing) 9bb368d Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing) e3497bb Separate between clearing memory and setting to zero in tests (Tim Ruffing) d79a6cc Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing) 1c08126 Add secp256k1_memclear() for clearing secret data (Tim Ruffing) e7d3844 Don't clear secrets in pippenger implementation (Tim Ruffing) Pull request description: This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master. Some changes to the original PR: * the clearing function now has the `secp256k1_` prefix again, since the related helper `_memczero` got it as well (see PR #835 / commit e89278f) * the original commit b17a7df ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR #943 (commit d49011f) * clearing of stack memory with `secp256k1_memclear` is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered) So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in #636 (comment)), happy to go deeper there if this gets Concept ACKed. The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. https://github.com/google/boringssl/blob/5af122c3dfc163b5d1859f1f450756e8e320a142/crypto/mem.c#L335 and https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/string.h#L348 / https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/compiler.h#L102 Fixes #185. ACKs for top commit: sipa: reACK 765ef53 real-or-random: ACK 765ef53 Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959

Don't use reserved identifiers memczero and benchmark_verify_t

e89278f

As identified in bitcoin-core#829 and bitcoin-core#833. Fixes bitcoin-core#829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent.

sipa reviewed Oct 27, 2020

View reviewed changes

src/util.h Show resolved Hide resolved

Typedef (u)int128_t only when they're not provided by the compiler

1f4dd03

real-or-random force-pushed the 202010-reserved-identifiers branch from 7d878a0 to 1f4dd03 Compare October 27, 2020 19:18

real-or-random requested a review from sipa October 29, 2020 11:19

jonasnick reviewed Nov 4, 2020

View reviewed changes

sipa merged commit 9e5939d into bitcoin-core:master Nov 4, 2020

theStack mentioned this pull request Aug 6, 2024

Clear sensitive memory without getting optimized out (revival of #636) #1579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use reserved identifiers memczero and benchmark_verify_t #835

Don't use reserved identifiers memczero and benchmark_verify_t #835

real-or-random commented Oct 20, 2020 •

edited

Loading

elichai commented Oct 20, 2020 •

edited

Loading

gmaxwell commented Oct 21, 2020

real-or-random commented Oct 26, 2020

elichai commented Oct 26, 2020

elichai commented Oct 26, 2020

real-or-random commented Oct 26, 2020

sipa commented Oct 26, 2020 •

edited

Loading

gmaxwell commented Oct 26, 2020

sipa commented Oct 27, 2020

real-or-random commented Oct 27, 2020

jonasnick left a comment

real-or-random commented Nov 4, 2020

jonasnick left a comment

sipa commented Nov 4, 2020

Don't use reserved identifiers memczero and benchmark_verify_t #835

Don't use reserved identifiers memczero and benchmark_verify_t #835

Conversation

real-or-random commented Oct 20, 2020 • edited Loading

elichai commented Oct 20, 2020 • edited Loading

gmaxwell commented Oct 21, 2020

real-or-random commented Oct 26, 2020

elichai commented Oct 26, 2020

elichai commented Oct 26, 2020

real-or-random commented Oct 26, 2020

sipa commented Oct 26, 2020 • edited Loading

gmaxwell commented Oct 26, 2020

sipa commented Oct 27, 2020

real-or-random commented Oct 27, 2020

jonasnick left a comment

Choose a reason for hiding this comment

real-or-random commented Nov 4, 2020

jonasnick left a comment

Choose a reason for hiding this comment

sipa commented Nov 4, 2020

real-or-random commented Oct 20, 2020 •

edited

Loading

elichai commented Oct 20, 2020 •

edited

Loading

sipa commented Oct 26, 2020 •

edited

Loading