Skip to content

Conversation

@niluxv
Copy link
Contributor

@niluxv niluxv commented May 2, 2021

Adds the following functions to platforms that support them:

  • explicit_bzero (de facto standard): Glibc, MUSL, FreeBSD, DragonFly BSD, OpenBSD
  • explicit_memset: NetBSD
  • memset_s (C11 standard): FreeBSD, DragonFly BSD, Apple OSX

These functions are useful for zeroing secret memory.

Closes #2009

Adds the following functions to platforms that support them:
* `explicit_bzero` (de facto standard): Glibc, MUSL, FreeBSD, DragonFly BSD, OpenBSD
* `explicit_memset`: NetBSD
* `memset_s` (C11 standard): FreeBSD, DragonFly BSD

These functions are useful for zeroing secret memory.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@niluxv
Copy link
Contributor Author

niluxv commented May 2, 2021

As for the failing FreeBSD tests: rsize_t is a size_t from the C11 standard annex K, and similarly errno_t is an int. Should I just replace them in the memset_s signature?
Edit: or just remove the memset_s bindings entirely as C11 annex K is not very popular (understatement), and the platforms that provide it also provide explicit_bzero.

// Added in `DragonFly BSD` 5.4
pub fn explicit_bzero(s: *mut ::c_void, len: ::size_t);
// ISO/IEC 9899:2011 ("ISO C11") K.3.7.4.1
pub fn memset_s(s: *mut ::c_void, smax: ::rsize_t, c: ::c_int, n: ::rsize_t) -> ::errno_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

macOs has also memset_s if that helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added memset_s to macOS and fixed the signature to use standard C types instead of the C11 annex K types that were used.

niluxv added 2 commits May 5, 2021 18:02
Use the standard C type equivalent to the C11 annex K types
pub fn iconv_close(cd: iconv_t) -> ::c_int;
}

extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you declare a new extern block instead of using the existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason I think. I started with glibc and there were already 3 different extern "C" blocks there (all without special link attributes), and I didn't know which one I should add this function to, so I added a extern "C" block there. And for the other platforms I copied my code from glibc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be an empty line between the added function(s) and the existing functions? In musl for example the functions seem to be grouped using empty lines in logical groups, while e.g. in OpenBSD there are no empty lines in the extern "C" block at all.

Copy link
Member

Choose a reason for hiding this comment

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

No particular reason I think. I started with glibc and there were already 3 different extern "C" blocks there (all without special link attributes), and I didn't know which one I should add this function to, so I added a extern "C" block there. And for the other platforms I copied my code from glibc.

I should've overlooked them, let's use one block here.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

LGTM. cc @semarie this touches the OpenBSD module.

@semarie
Copy link
Contributor

semarie commented May 11, 2021

@JohnTitor hep, read fine for openbsd. thanks

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 11, 2021

📌 Commit ea18049 has been approved by JohnTitor

@bors
Copy link
Contributor

bors commented May 11, 2021

⌛ Testing commit ea18049 with merge d3468b2...

@bors
Copy link
Contributor

bors commented May 11, 2021

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: JohnTitor
Pushing d3468b2 to master...

@bors bors merged commit d3468b2 into rust-lang:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add memset_s and/or explicit_bzero for platforms that support it

7 participants