Add getrandom impl using ctru_sys bindings #8

ian-h-chamberlain · 2022-01-30T23:16:40Z

Add a getrandom call that libc can link against. This will also require a change to libc to declare an extern "C" fn getrandom and then finally update std to utilize that call when target_os = "horizon".

Two main questions:

Is this the right place to put this impl? we could put it in ctru-sys instead if we wanted, but putting it here feels like a "linker-fix" to me. It does require adding the dependency on ctru-sys, though.
How do we want to handle conversion errors? try_into().unwrap() feels a bit heavy-handed, maybe if the conversion fails we should set errno or something? Does ctru-sys have an API for something like errno for us to set in case of non-zero return values from the PS_ syscall?

@Meziu @AzureMarker

Meziu · 2022-01-30T23:18:48Z

Make a PR to libc too.

Meziu · 2022-01-30T23:23:57Z

Is this the right place to put this impl?

Yes, this is a standard Newlib function, and it's a linker fix for it since it's not normally available.

AzureMarker · 2022-01-31T00:25:44Z

At some point we should consider renaming the linker fix lib to something like ctru-libc-polyfill.

AzureMarker · 2022-01-31T00:29:29Z

src/lib.rs

+    buflen: libc::size_t,
+    _flags: libc::c_uint,
+) -> libc::ssize_t {
+    let ret = ctru_sys::psInit();


Does this need to happen every time, or can we lazily initialize it with something like core::sync::Once?

Also, do we need to call psExit or not worry about it?

Good question re: psExit. The docs are pretty sparse on what these functions actually do, but I would assume that it is more correct to call it before the program ends. I'm not really sure how we could handle something like that in this crate... maybe this impl should only call PS_GenerateRandomBytes and it's up to the user to initialize / exit PS itself?

Oh, also – as far as I can tell there's no problem calling it more than once, but it could be done with Once if we wanted to avoid the potential overhead

If it's OK with not calling psExit then I'd go with the Once route.

I opted to remove psInit entirely, as I think it makes more sense to treat Ps the way e.g. Gfx and Apt are in ctru-rs already (and I have a local changeset to do so). With the new implementation I pushed, failing to initialize the Ps causes a runtime panic (from std) if the user tries to e.g. construct a HashMap, but everything works if they do initialize it.

src/lib.rs

ian-h-chamberlain · 2022-01-31T02:43:43Z

Trying it out with a real application (HashMap), and I think there is some additional work to make it work as expected, e.g. https://man7.org/linux/man-pages/man2/getrandom.2.html#RETURN_VALUE

ian-h-chamberlain · 2022-01-31T03:55:56Z

src/lib.rs

+    mut buflen: libc::size_t,
+    flags: libc::c_uint,
+) -> libc::ssize_t {
+    // TODO: is this needed? Maybe just `buflen = buflen.min(libc::ssize_t::MAX)` ?


This number chosen based on https://man7.org/linux/man-pages/man2/getrandom.2.html#NOTES

There is the GRND_RANDOM flag for using random instead of urandom(under linux). Maybe we could check those to change the maximum buffer length(for urandom it's 33554431, while for random it's 512), so it would more closely resemble the original.

GRND_NONBLOCK instead looks like it can just be ignored, as it doesn't change how our code behaves.

We don't need to return errors when those flags are set.

Interesting... as it turns out, getrandom, getentropy, GRND_NONBLOCK, and GRND_RANDOM seem to only be defined in newlib for Cygwin, and not on 3DS (or any other platform).

So, I guess I will just add them to horizon/mod.rs in libc, and anyone using it will just have to live with the fact that GRND_NONBLOCK doesn't work at all.

src/lib.rs

AzureMarker · 2022-02-01T01:53:19Z

src/lib.rs

+    let ret = ctru_sys::PS_GenerateRandomBytes(buf, buflen as libc::c_uint) as libc::ssize_t;
+    if ret < 0 {
+        // this is kind of a hack, but at least gives some visibility to the
+        // error code returned by PS_GenerateRandomBytes I guess? Another option


Based on this header file, these error codes aren't compatible:
https://github.com/devkitPro/libctru/blob/master/libctru/include/3ds/result.h

We would probably want to parse the 3ds result and return the appropriate error code.

Yeah, std implementation seems to just report them as "unexpected error code XYZ" if I set it in errno here. I have some local changes to sort of "parse" those error codes based on https://www.3dbrew.org/wiki/Error_codes but seeing that header, I wonder if we can just leverage ports of those macros instead. Let me see if I can put something in ctru-sys for this...

Looks nice! 👍

ian-h-chamberlain · 2022-02-01T04:06:24Z

Okay, I've updated all relevant PRs for getrandom with what I think makes sense and based on your comments. Let me know if any further changes are necessary, in particular I think the ctru_sys::Result -> errno code is not perfect, but I'm not sure what else we can do since there is not a 1-to-1 translation.

Meziu

Looks good! Will merge once rust3ds/ctru-rs#35 is finalized.

Meziu · 2022-02-01T17:32:17Z

OH, having linker-fix depend on ctru-sys creates a dependency duplication error in cargo. @ian-h-chamberlain

ian-h-chamberlain · 2022-02-01T17:34:01Z

OH, having linker-fix depend kn ctru-sys creates a dependency duplication error in cargo. @ian-h-chamberlain

Ah yeah, I think I didn't notice because I had so many [patch]es going during testing. I think it's easy enough to result by adding one, let me see...

Meziu · 2022-02-01T17:37:56Z

Fixed in rust3ds/ctru-rs@c59de27 @ian-h-chamberlain

It is cool to have that link field, but ultimately useless, since the library is linked by cargo-3ds at the top level.

ian-h-chamberlain · 2022-02-01T17:49:54Z

Fixed in Meziu/ctru-rs@c59de27 @ian-h-chamberlain

It is cool to have that link field, but ultimately useless, since the library is linked by cargo-3ds at the top level.

That's true, I think technically we could remove it from cargo-3ds if we left it in the ctru-sys Cargo.toml. We may still want the [patch], since otherwise there could be confusing issues in the dependency resolution:

$ cargo tree 
ctru-rs v0.7.1 (/Users/ianchamberlain/Documents/Development/3ds/ctru-rs/ctru-rs)
├── bitflags v1.3.2
├── cfg-if v1.0.0
├── const-zero v0.1.0
├── ctru-sys v0.4.0 (/Users/ianchamberlain/Documents/Development/3ds/ctru-rs/ctru-sys)
│   └── libc v0.2.116 (https://github.com/Meziu/libc.git#ab957c0c)
├── libc v0.2.116 (https://github.com/Meziu/libc.git#ab957c0c)
├── linker-fix-3ds v0.1.0 (https://github.com/Meziu/rust-linker-fix-3ds.git#f6c3cde6)
│   ├── ctru-sys v0.4.0 (https://github.com/Meziu/ctru-rs.git#c59de27f)
│   │   └── libc v0.2.116 (https://github.com/Meziu/libc.git#ab957c0c)
│   └── libc v0.2.116 (https://github.com/Meziu/libc.git#ab957c0c)
...

See how there are two different versions of ctru-sys?

Meziu · 2022-02-01T17:53:51Z

Yes, absolutely, dependency duplicates aren't cool but link = ctru can't stay anyways. ctru wouldn't link to the standard otherwise.

Add getrandom impl using ctru_sys bindings

f5d2221

ian-h-chamberlain mentioned this pull request Jan 31, 2022

Add getrandom call on horizon OS Meziu/libc#3

Merged

AzureMarker reviewed Jan 31, 2022

View reviewed changes

Match more typical getrandom semantics

af7d34f

ian-h-chamberlain commented Jan 31, 2022

View reviewed changes

ian-h-chamberlain mentioned this pull request Jan 31, 2022

Enable getrandom on horizon Meziu/rust-horizon#7

Closed

AzureMarker reviewed Feb 1, 2022

View reviewed changes

ian-h-chamberlain mentioned this pull request Feb 1, 2022

Add translations of 3DS result macros + debug impl rust3ds/ctru-rs#35

Merged

Update getrandom impl based on ctru error codes

754afa8

ian-h-chamberlain requested a review from Meziu February 1, 2022 04:06

ian-h-chamberlain mentioned this pull request Feb 1, 2022

Support CLOCK_MONOTONIC in clock_gettime #9

Closed

Meziu approved these changes Feb 1, 2022

View reviewed changes

Meziu merged commit f6c3cde into rust3ds:master Feb 1, 2022

rust3ds locked and limited conversation to collaborators Feb 1, 2022

rust3ds unlocked this conversation Feb 1, 2022

ian-h-chamberlain mentioned this pull request Feb 1, 2022

Add getrandom constants Meziu/libc#4

Merged

Meziu mentioned this pull request Mar 23, 2023

Naming of this crate / repository #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getrandom impl using ctru_sys bindings #8

Add getrandom impl using ctru_sys bindings #8

ian-h-chamberlain commented Jan 30, 2022 •

edited

Loading

Meziu commented Jan 30, 2022

Meziu commented Jan 30, 2022

AzureMarker commented Jan 31, 2022

AzureMarker Jan 31, 2022

ian-h-chamberlain Jan 31, 2022

ian-h-chamberlain Jan 31, 2022

AzureMarker Jan 31, 2022 •

edited

Loading

ian-h-chamberlain Jan 31, 2022 •

edited

Loading

ian-h-chamberlain commented Jan 31, 2022

ian-h-chamberlain Jan 31, 2022

Meziu Jan 31, 2022 •

edited

Loading

ian-h-chamberlain Feb 1, 2022

AzureMarker Feb 1, 2022

ian-h-chamberlain Feb 1, 2022

AzureMarker Feb 1, 2022

ian-h-chamberlain commented Feb 1, 2022

Meziu left a comment

Meziu commented Feb 1, 2022 •

edited

Loading

ian-h-chamberlain commented Feb 1, 2022

Meziu commented Feb 1, 2022 •

edited

Loading

ian-h-chamberlain commented Feb 1, 2022

Meziu commented Feb 1, 2022

Add getrandom impl using ctru_sys bindings #8

Add getrandom impl using ctru_sys bindings #8

Conversation

ian-h-chamberlain commented Jan 30, 2022 • edited Loading

Meziu commented Jan 30, 2022

Meziu commented Jan 30, 2022

AzureMarker commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AzureMarker Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

ian-h-chamberlain Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

ian-h-chamberlain commented Jan 31, 2022

Choose a reason for hiding this comment

Meziu Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-h-chamberlain commented Feb 1, 2022

Meziu left a comment

Choose a reason for hiding this comment

Meziu commented Feb 1, 2022 • edited Loading

ian-h-chamberlain commented Feb 1, 2022

Meziu commented Feb 1, 2022 • edited Loading

ian-h-chamberlain commented Feb 1, 2022

Meziu commented Feb 1, 2022

ian-h-chamberlain commented Jan 30, 2022 •

edited

Loading

AzureMarker Jan 31, 2022 •

edited

Loading

ian-h-chamberlain Jan 31, 2022 •

edited

Loading

Meziu Jan 31, 2022 •

edited

Loading

Meziu commented Feb 1, 2022 •

edited

Loading

Meziu commented Feb 1, 2022 •

edited

Loading