Get software keyboard input without max byte size parameter #157

FenrirWolf · 2024-02-12T00:49:24Z

Ever since I first started messing with the software keyboard from Rust, I've always hated that libctru's swkbd API forces you to use a fixed-size buffer in order to receive string data from the keyboard. We've worked around that restriction by just forcing the user to specify a maximum size for the intermediate text buffer, but I've always hated that solution too. And so I randomly decided it's finally time to do something about it.

However, it turns out that "doing something about it" involves reimplementing both swkbdInputText and swkbdMessageCallback in Rust and handling a whole bunch of gnarly unsafe code ourselves instead of just leaving all of that nonsense to libctru.

Is it worth introducing this much unsafe code just to solve a small API nitpick? Frankly I'm not entirely sure, but I thought I'd throw this out there and see what you guys think.

Meziu · 2024-02-14T18:24:05Z

Wow! This re-implementation looks quite nice. I must say I'm not against the "amounts" of unsafe code present here, since it manages to strengthen the API quite a lot and give us more control over the internal implementation (as I said in the past, especially regarding ndsp, there is a lot of things we could improve by getting our hands a bit dirty).

I'll try a deep dive review once I manage to get some time.

Is this much unsafe code worth such a small quality of life change? Maybe!

Isolating unsafe code is still important even in unsafe functions

This lets us mark the function as safe since it can only be called with an initialized `SwkbdState`

FenrirWolf · 2024-02-16T22:41:45Z

Okay, I think this PR is in a pretty good state now and I probably won't need to push any more changes. I'm not sure what the nightly test failures are about though. They don't seem related to anything actually going on in the code here.

ian-h-chamberlain

Mostly LGTM, had some small comments / questions about some of the implementation but a lot of it is just musing about future possibilities and I don't think anything blocking.

IMO probably would be fine to merge as-is with one more approval

ian-h-chamberlain · 2024-02-16T23:49:54Z

ctru-rs/src/applets/swkbd.rs

+struct MessageCallbackData {
+    extra: *mut SwkbdExtra,
+    swkbd_shared_mem_ptr: *mut libc::c_void,
+}


Is this something that we might eventually be able to generalize to a MessageCallbackData<T> where T = SwkbdExtra in this case? I know some people have been interested in general-purpose svc helpers to work with system calls that don't have "nice" APIs in libctru, so just wondering how generic this code is.

Definitely not asking for something like that in this PR, but just wondering if this PR might provide a good starting point from which that could be built. I believe some of the ir_user module by @AzureMarker also did a bit of svc memory allocation and stuff but I am not familiar with the specifics so not sure if there is any overlap there.

I only made this struct because libctru doesn't include a pointer to the shared memory block in SwkbdExtra like it ought to, and instead uses a static to pass the pointer into the APT message callback. I decided there's no reason we have to do the same in our implementation.

I've never messed with any other APT message calls, so I'm not sure if this specific struct would be helpful outside of this particular context.

ian-h-chamberlain · 2024-02-16T23:59:09Z

ctru-rs/src/applets/swkbd.rs

+        shared_mem_size +=
+            (std::mem::size_of::<u16>() * (swkbd.max_text_len as usize + 1) + 3) & !3;


I assume the + 3) & !3 is an alignment thing (align to usize, maybe)? does it make sense to have something like this just to make the math more obvious?

let max_len = swkbd.max_text_len as usize + 1; shared_mem_size += (size_of::<u16>() * max_len).next_multiple_of(size_of::<usize>()); // etc.

Sure, that might make more sense. I'm honestly not very fluent in bitwise math, so I just kinda translated the C code directly without getting too fancy about interpreting its meaning.

ian-h-chamberlain · 2024-02-17T00:05:07Z

ctru-rs/src/applets/swkbd.rs

+        if swkbd.save_state_flags & (1 << 0) != 0 {
+            swkbd.status_offset = shared_mem_size as _;
+            shared_mem_size += std::mem::size_of::<SwkbdStatusData>();
+        }
+
+        if swkbd.save_state_flags & (1 << 1) != 0 {
+            swkbd.learning_offset = shared_mem_size as _;
+            shared_mem_size += std::mem::size_of::<SwkbdLearningData>();
+        }


It would be nice if we could use bitflags for this but I think this struct is defined by ctru_sys right? so maybe there's no great option but maybe defining consts for the bitshifts would be useful

ian-h-chamberlain · 2024-02-17T00:05:46Z

ctru-rs/src/applets/swkbd.rs

+            shared_mem_size += std::mem::size_of::<SwkbdLearningData>();
+        }
+
+        shared_mem_size = (shared_mem_size + 0xFFF) & !0xFFF;


similar comment, could this be

swkbd.shared_memory_size = shared_mem_size.next_multiple_of(0x1000);

ian-h-chamberlain · 2024-02-17T00:09:45Z

ctru-rs/src/applets/swkbd.rs

+        swkbd.shared_memory_size = shared_mem_size;
+
+        // Allocate shared mem
+        let swkbd_shared_mem_ptr = unsafe { libc::memalign(0x1000, shared_mem_size) };


huh, I was thinking it would be nice to use Rust alloc for this but I guess it's passed to C immediately anyway so there's not really a compelling reason to do that that I can think of.

I did think about using std::alloc, but as you said the memory block is only being manipulated by C code anyway and so it's more straightforward to just use memalign and free

ctru-rs/src/applets/swkbd.rs

ian-h-chamberlain · 2024-02-17T00:39:17Z

ctru-rs/src/applets/swkbd.rs

+
+        if swkbd.text_length > 0 {
+            let text16 = unsafe {
+                widestring::Utf16Str::from_slice_unchecked(std::slice::from_raw_parts(


I assume the _unchecked here means that we are trusting either the OS or libctru to give us a valid string, right? Just wondering if an extra check here might be worth a little bit of safety

Yeah, we're trusting the Software Keyboard to give us proper UTF-16 text here and I don't think there's any reason to suspect it would fail at that job

ian-h-chamberlain · 2024-02-17T00:41:40Z

ctru-rs/src/applets/swkbd.rs

+                    swkbd_shared_mem_ptr.add(swkbd.learning_offset as _).cast(),
+                    extra.learning_data,
+                    1,
+                )


nit: might increase readibility here to use the method form:

extra.learning_data.copy_from_nonoverlapping( swkbd_shared_mem_ptr.add(swkbd.learning_offset as _).cast(), 1, );

ian-h-chamberlain · 2024-02-17T00:46:05Z

ctru-rs/src/applets/swkbd.rs

+            };
+        }
+
+        unsafe { libc::free(swkbd_shared_mem_ptr) };


Allocating and freeing all the options every time is probably fine, and I don't think we need to optimize right now — but I wonder if we could also have more of a Builder API pattern that sets all the sizing options up front and allocates just once, which would probably make this call cheaper.
Don't want to prematurely optimize though since swkbd isn't likely to be a hot path for most apps (I hope, lol!)

Yeah, calling the software keyboard involves a lot of setup no matter how you slice it. I'm not sure if it would be worth trying to reduce allocations here, especially since any changes to the software keyboard config would require recomputing and reallocating the memory block anyway.

ian-h-chamberlain · 2024-02-17T00:47:17Z

ctru-rs/src/applets/swkbd.rs

+    // A reimplementation of `swkbdMessageCallback` from `libctru/source/applets/swkbd.c`.
+    // This is only needed because the original function is private to libctru, so we can't
+    // simply reuse their version
+    #[deny(unsafe_op_in_unsafe_fn)]


❤️❤️❤️

I have sometimes considered if we should consider denying as a library lint but I think it would be a nontrivial amount of work so never bothered to try

I am in favour of this as well! Unsafe blocks should be marked only when necessary and not all unsafe functions use unsafe code (especially around here...).

FenrirWolf · 2024-02-17T02:52:48Z

Also while we're messing around in here, what do you guys think about renaming SoftwareKeyboard::get_string to SoftwareKeyboard::launch? I only chose the former name back when there were 2 functions for getting text data from the keyboard and I wanted to make sure there was a strong distinction between the two, but since this PR does away with the write_exact method, I feel like that would be a sensible change

ian-h-chamberlain · 2024-02-18T18:18:24Z

Also while we're messing around in here, what do you guys think about renaming SoftwareKeyboard::get_string to SoftwareKeyboard::launch?

Works for me

Meziu

LGTM. The only things I'd like to see improved are the alignment calculations that @ian-h-chamberlain pointed out. Other than that, it's fine.

FenrirWolf · 2024-02-20T17:53:43Z

Apparently next_multiple_of is only stable as of rust 1.73 and nightly-2023-06-01 is older than that. Everything works on my end though.

Needed after rust3ds/ctru-rs#157

FenrirWolf requested a review from a team as a code owner February 12, 2024 00:49

FenrirWolf requested review from AzureMarker, ian-h-chamberlain and Meziu February 12, 2024 00:49

FenrirWolf force-pushed the software-keyboard-shenanigans branch 5 times, most recently from ab8d86d to 65f7d26 Compare February 14, 2024 06:48

FenrirWolf force-pushed the software-keyboard-shenanigans branch 3 times, most recently from 890e226 to e593abd Compare February 14, 2024 23:33

FenrirWolf mentioned this pull request Feb 15, 2024

Improve SoftwareKeyboard::set_filter_callback #160

Closed

FenrirWolf added 15 commits February 16, 2024 11:19

Get swkbd input without max byte size parameter

742ec8d

Is this much unsafe code worth such a small quality of life change? Maybe!

Don't include nul terminator when passing string data back to Rust

2864c06

Use .is_null() instead of comparing to std::ptr::null()

5413e77

Use references and make unsafe blocks more fine-grained

3a2000f

Use rust functions instead of memcpy and memset

bd19f97

Simplify string conversion code

fa6f98d

Use if let instead of unwrap for nullable callback

f867d96

Remove mention of deleted function

61f9131

swkbd_input_text is still an unsafe fn

4de7652

Simplify string output logic

2fcdd11

Fix off-by-one error in swkbd_message_callback

f527623

Simplify some code + rename vars

b6fc9a2

Force consistent use of unsafe blocks

bf3ca2a

Isolating unsafe code is still important even in unsafe functions

Fix yet another off-by-one error

271d1b5

Make swkbd_input_text a method for SoftwareKeyboard

757afe9

This lets us mark the function as safe since it can only be called with an initialized `SwkbdState`

FenrirWolf force-pushed the software-keyboard-shenanigans branch from 0e0b9ae to 757afe9 Compare February 16, 2024 18:28

FenrirWolf force-pushed the software-keyboard-shenanigans branch from 0eb4efe to 73e5628 Compare February 16, 2024 21:09

Get rid of static mut by using our own callback data

cfd276c

FenrirWolf force-pushed the software-keyboard-shenanigans branch from 73e5628 to cfd276c Compare February 17, 2024 00:00

ian-h-chamberlain approved these changes Feb 17, 2024

View reviewed changes

FenrirWolf added 3 commits February 16, 2024 19:34

code_point -> code_unit

03b417d

Use .cast() in more places

d6f22dc

Add some comments

f715fc6

MessageCallbackData doesn't need to be a struct member

1446583

FenrirWolf force-pushed the software-keyboard-shenanigans branch from 17f07c5 to 1446583 Compare February 17, 2024 19:16

ian-h-chamberlain approved these changes Feb 18, 2024

View reviewed changes

FenrirWolf added 2 commits February 19, 2024 16:11

Remove redudant imports

0576e34

SoftwareKeyboard::get_string -> SoftwareKeyboard::launch

cb67d92

Meziu approved these changes Feb 20, 2024

View reviewed changes

Use next_multiple_of instead of bitshifts

9c33e49

FenrirWolf added 2 commits February 20, 2024 10:57

Update CI nightly version

6d6f9c0

Update MSRV to 1.73

636c8d7

FenrirWolf merged commit 0345f49 into rust3ds:master Feb 20, 2024
2 of 4 checks passed

FenrirWolf deleted the software-keyboard-shenanigans branch February 20, 2024 22:31

ian-h-chamberlain added a commit to rust3ds/actions that referenced this pull request Feb 22, 2024

Update nightly Rust version

98a8ed4

Needed after rust3ds/ctru-rs#157

FenrirWolf mentioned this pull request Feb 23, 2024

Require unsafe blocks in unsafe fns #165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get software keyboard input without max byte size parameter #157

Get software keyboard input without max byte size parameter #157

FenrirWolf commented Feb 12, 2024

Meziu commented Feb 14, 2024

FenrirWolf commented Feb 16, 2024 •

edited

ian-h-chamberlain left a comment

ian-h-chamberlain Feb 16, 2024

FenrirWolf Feb 17, 2024 •

edited

ian-h-chamberlain Feb 16, 2024

FenrirWolf Feb 17, 2024 •

edited

ian-h-chamberlain Feb 17, 2024

ian-h-chamberlain Feb 17, 2024

ian-h-chamberlain Feb 17, 2024

FenrirWolf Feb 17, 2024

ian-h-chamberlain Feb 17, 2024

FenrirWolf Feb 17, 2024 •

edited

ian-h-chamberlain Feb 17, 2024

ian-h-chamberlain Feb 17, 2024

FenrirWolf Feb 17, 2024

ian-h-chamberlain Feb 17, 2024

Meziu Feb 20, 2024

FenrirWolf commented Feb 17, 2024

ian-h-chamberlain commented Feb 18, 2024

Meziu left a comment

FenrirWolf commented Feb 20, 2024

		shared_mem_size +=
		(std::mem::size_of::<u16>() * (swkbd.max_text_len as usize + 1) + 3) & !3;

Get software keyboard input without max byte size parameter #157

Get software keyboard input without max byte size parameter #157

Conversation

FenrirWolf commented Feb 12, 2024

Meziu commented Feb 14, 2024

FenrirWolf commented Feb 16, 2024 • edited

ian-h-chamberlain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FenrirWolf Feb 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FenrirWolf Feb 17, 2024 • edited

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

Choose a reason for hiding this comment

FenrirWolf Feb 17, 2024 • edited

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

Choose a reason for hiding this comment

FenrirWolf commented Feb 17, 2024

ian-h-chamberlain commented Feb 18, 2024

Meziu left a comment

Choose a reason for hiding this comment

FenrirWolf commented Feb 20, 2024

FenrirWolf commented Feb 16, 2024 •

edited

FenrirWolf Feb 17, 2024 •

edited

FenrirWolf Feb 17, 2024 •

edited

FenrirWolf Feb 17, 2024 •

edited