| Message ID | 20230824-strncpy-drivers-acpi-acpica-v1-1-d027ba183b66@google.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] ACPICA: remove acpi_ut_safe_strncpy in favor of strscpy | expand |
On Thu, Aug 24, 2023 at 10:02:02PM +0000, Justin Stitt wrote: > I wanted to gather some thoughts on removing `acpi_ut_safe_strncpy` (and > potentially other `acpi...safe...()` interfaces) in favor of > pre-existing interfaces in the kernel (like strscpy). > > Running a git blame shows these functions were implemented 10 years ago > and their implementations generally mirror the _newer_ and more robust > stuff in lib/string.h -- Let's just use these, right? > > I appreciate any comments and whether or not I should stop at just > `strncpy`. ACPICA is actually a separate upstream project, so changes are best made there[1]. However, this code base is shared with many OSes and compilers, so there won't be a common "strscpy" available. Perhaps the right thing to do here is to implement acpi_ut_safe_strncpy() in terms of strnlen(), memcpy(), and memset(). That would make the upstream project safe against "too long reads", etc, and would require no collateral changes: void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size) { /* Do not over-read the source string. */ acpi_size len = 0; if (dest_size > 0) len = strnlen(source, dest_size - 1); if (len) memcpy(dest, source, len) /* Always terminate destination string and pad to dest_size. */ memset(dest + len, '\0', dest_size - len); } -Kees [1] https://github.com/acpica/acpica e.g. https://github.com/acpica/acpica/pull/856
diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h index edfdbbef81c1..4d5c401a14b4 100644 --- a/drivers/acpi/acpica/acutils.h +++ b/drivers/acpi/acpica/acutils.h @@ -626,8 +626,6 @@ void acpi_ut_repair_name(char *name); #if defined (ACPI_DEBUGGER) || defined (ACPI_APPLICATION) || defined (ACPI_DEBUG_OUTPUT) u8 acpi_ut_safe_strcpy(char *dest, acpi_size dest_size, char *source); -void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size); - u8 acpi_ut_safe_strcat(char *dest, acpi_size dest_size, char *source); u8 diff --git a/drivers/acpi/acpica/dbfileio.c b/drivers/acpi/acpica/dbfileio.c index e1b6e54a96ac..197e1ff1f72c 100644 --- a/drivers/acpi/acpica/dbfileio.c +++ b/drivers/acpi/acpica/dbfileio.c @@ -63,7 +63,7 @@ void acpi_db_open_debug_file(char *name) } acpi_os_printf("Debug output file %s opened\n", name); - acpi_ut_safe_strncpy(acpi_gbl_db_debug_filename, name, + strscpy(acpi_gbl_db_debug_filename, name, sizeof(acpi_gbl_db_debug_filename)); acpi_gbl_db_output_to_file = TRUE; } diff --git a/drivers/acpi/acpica/psutils.c b/drivers/acpi/acpica/psutils.c index d550c4af4702..95af2c74e00b 100644 --- a/drivers/acpi/acpica/psutils.c +++ b/drivers/acpi/acpica/psutils.c @@ -60,7 +60,7 @@ void acpi_ps_init_op(union acpi_parse_object *op, u16 opcode) op->common.descriptor_type = ACPI_DESC_TYPE_PARSER; op->common.aml_opcode = opcode; - ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name, + ACPI_DISASM_ONLY_MEMBERS(strscpy(op->common.aml_op_name, (acpi_ps_get_opcode_info (opcode))->name, sizeof(op->common. diff --git a/drivers/acpi/acpica/utnonansi.c b/drivers/acpi/acpica/utnonansi.c index ff0802ace19b..a465e5a1d309 100644 --- a/drivers/acpi/acpica/utnonansi.c +++ b/drivers/acpi/acpica/utnonansi.c @@ -164,12 +164,4 @@ acpi_ut_safe_strncat(char *dest, return (FALSE); } -void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size) -{ - /* Always terminate destination string */ - - strncpy(dest, source, dest_size); - dest[dest_size - 1] = 0; -} - #endif diff --git a/drivers/acpi/acpica/uttrack.c b/drivers/acpi/acpica/uttrack.c index f5f5da441458..a96a86bc5781 100644 --- a/drivers/acpi/acpica/uttrack.c +++ b/drivers/acpi/acpica/uttrack.c @@ -368,7 +368,7 @@ acpi_ut_track_allocation(struct acpi_debug_mem_block *allocation, allocation->component = component; allocation->line = line; - acpi_ut_safe_strncpy(allocation->module, (char *)module, + strscpy(allocation->module, (char *)module, ACPI_MAX_MODULE_NAME); if (!element) {
I wanted to gather some thoughts on removing `acpi_ut_safe_strncpy` (and potentially other `acpi...safe...()` interfaces) in favor of pre-existing interfaces in the kernel (like strscpy). Running a git blame shows these functions were implemented 10 years ago and their implementations generally mirror the _newer_ and more robust stuff in lib/string.h -- Let's just use these, right? I appreciate any comments and whether or not I should stop at just `strncpy`. Thanks Signed-off-by: Justin Stitt <justinstitt@google.com> --- drivers/acpi/acpica/acutils.h | 2 -- drivers/acpi/acpica/dbfileio.c | 2 +- drivers/acpi/acpica/psutils.c | 2 +- drivers/acpi/acpica/utnonansi.c | 8 -------- drivers/acpi/acpica/uttrack.c | 2 +- 5 files changed, 3 insertions(+), 13 deletions(-) --- base-commit: f9604036a3fb6149badf346994b46b03f9292db7 change-id: 20230824-strncpy-drivers-acpi-acpica-321af3eb414e Best regards, -- Justin Stitt <justinstitt@google.com>