rhboot / shim

UEFI shim loader
Other
819 stars 289 forks source link

Rename 'msecs' to 'usecs' to avoid potential confusion #563

Closed aronowski closed 1 year ago

aronowski commented 1 year ago

As suggested in https://github.com/rhboot/shim/issues/251.

The function msleep uses gBS->Stall which waits for a specified number of microseconds.

Reference: https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/51_services_that_uefi_drivers_commonly_use/517_stall

This reference even mentions an example slepping for 10 microseconds: // Wait 10 uS. Notice the letter 'u'.

Therefore it's a good idea to call the argument msleep uses 'usecs' rather than 'msecs' so no one confuses it with miliseconds.

dennis-tseng99 commented 1 year ago

Normally, parameter usecs is used for usleep(useconds_t usec), and msecs is used for msleep(unsigned int msecs). Hence, to avoid confusion, could we also change the function name from msleep() to usleep() ?

aronowski commented 1 year ago

I like this idea but the implementation of it may be more complex than I initially thought.

I'm not a developer as of today and I'm unsure if such a change won't break anything crucial. I don't want to introduce major changes especially for a security-critical component such as Shim.

Changing argument names only resulted in the exact same binary, while changing function names results in a binary with a different checksum. While it works fine and allows for booting a system with Secure Boot enabled, I'm unsure if I should do this.

Maybe I'll let the Red Hat Bootloader Team decide on this.

vathpela commented 1 year ago

The function name should be changed as well.

dennis-tseng99 commented 1 year ago

@aronowski If you don't mind, please let me change the function name and its argument as well in another PR according to vathpela's suggestion.

aronowski commented 1 year ago

The function has been renamed according to Peter's request.

@dennis-tseng99 I did this change once I saw my first email notification rather than following the complete conversation. You can, however, review the latest commit so I can be sure I haven't made any mistake.

vathpela commented 1 year ago

I think wrapping the decl in a test for SHIM_UNIT_TEST should fix the unit test failures:

diff --git a/include/console.h b/include/console.h
index 04f02da7e01..8eb4b47f912 100644
--- a/include/console.h
+++ b/include/console.h
@@ -122,7 +122,9 @@ extern EFI_STATUS EFIAPI vdprint_(const CHAR16 *fmt, const char *file, int line,
 extern EFI_STATUS print_crypto_errors(EFI_STATUS rc, char *file, const char *func, int line);
 #define crypterr(rc) print_crypto_errors((rc), __FILE__, __func__, __LINE__)

+#ifndef SHIM_UNIT_TEST
 extern VOID usleep(unsigned long usecs);
+#endif

 /* This is used in various things to determine if we should print to the
  * console */
aronowski commented 1 year ago

@dennis-tseng99 @vathpela Thanks a lot for the hints. I've added the two changes. In both cases I've confirmed in my public fork that unit tests pass.

aronowski commented 1 year ago

Sure thing. It's right there! :grinning:

vathpela commented 1 year ago

I've squashed this up and re-ordered it a bit so that each commit passes the tests, and pushed it as 549d34691d68518e55c2edd6e759b19de7f8ddef and 908c388c922c6369cace0b76660198becee2284e.