Safer alternatives for "strncpy"

[kisslorand]

Requirements

BTT or MKS TFT

Description

This PR is about "strncpy()" function.
There's an endless controversy about its usage, most folks advise against the use of it, it's not safe and it can lead to very hard to detect bugs. Its major flaw is that it doesn't null-terminate the destination string so it's the programmer's duty to always make sure the resulting string is always null-terminated. It's easier to say than do it.
Let's say it is used in some function/procedure/subroutine and at that point it is safe to use that function, no danger of having a string not being null-terminated. Being an open source, some other programmer might use that function/subroutine in a new
feature he's developing and not checking every existing function/subroutine he's using along and use the said function with parameters that triggers the resulting string not being null-terminated. Another scenario could be the changing of a parameter/variable size due to some new functions/features that need that parameter being bigger in size, a parameter that is used somewhere along the code in that function/subroutine that uses "strncpy" and because of the enlarged size it will result in a non null-terminated string.
I think it is obvious why a non null-terminated string is screaming for trouble so I won't stress on explaining it.
Even the best of us can slip it over, just one example would be the "statusScreen_setMsg()" function which when called from "parseACK()" than data from a variable which can have 512 bytes of data (dmaL2Cache) is copied to a variable (msgBody) with the maximum size of 70 bytes. No measures are taken that the eventually truncated result is not null-terminated. It's that easy to fall into the trap of the function "strncpy()".

This PR introduces 3 alternatives for "strncpy()":

1. strxcpy(char * destination, const char * source, size_t num):

• this one always copy num-1 characters from source to destination regardless of null terminating character found in source
• destination always ends with '\0'

2. strwcpy(char * destination, const char * source, size_t width):

• this one copies the contents of the source to destination but no more than width-1 characters
• if null terminating character is found in the source than the rest in the destination is padded with 0
• destination always ends with '\0'

3. strscpy(char * destination, const char * source, size_t size):

• this one copies the contents of the source to destination but no more than size-1 characters
• if null terminating character is found in the source than the copy stops there
• destination always ends with '\0'

These 3 versions do not return anything, I saw it pointless to inherit the return a pointer to the destination since it is something that is already known at the time of calling these functions.

Benefits

• safer alternative for "strncpy()" by always resulting a null-terminated string
• lift the weight from the programmer's shoulder to always make sure to have a null-terminated string
• reduces compiled flash size

Notes

With this PR, if "strncopy()" is used in the code, at compilation time there will be a compilation error together with suggestions to use instead the above alternatives. This behaviour is intended, not accidental.

[bigtreetech]

I'm not sure if memcpy is safe when const char *src is NULL, so I will refactor it and submit commit for review later

[bigtreetech]

strscpy refer to the implementation of the linux kernel, and the parameters and return values are consistent with the general api

[bigtreetech]

@kisslorand I see you mark it as draft, do you have any new ideas?

[kisslorand]

I see problems with the new approach but Iam very busy at the moment. I marked it as draft to prevent merging it. Will come back with more details later today.  Sent from Yahoo Mail on Android On Fri, Feb 10, 2023 at 11:00, ***@***.***> wrote: @kisslorand I see you mark it as draft, do you have any new ideas? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[bigtreetech]

Got it

[kisslorand]

@kisslorand I see you mark it as draft, do you have any new ideas?

I do not have any new ideas but observations on latest commit (5bf70c8).
The new implementation of "strscpy" is the same as I did but with some more bells and whistles, additions that are not needed. The original "strncpy" doesn't check neither if source and destination are NULL pointers or not, this is not needed in this project, all the cases where strncpy is used originally uses initialized "strings". My implementation also handled just well if "size" parameter was zero. Current implementation increases flash size and doesn't fit this project in all the places it is uses it. For example it breaks the hiding of extensions of the G-code files in the file list. If extensions are selected to be hidden, they are still shown and if you select a file to be printed than a filename with a garbage at its tail is shown. Of course no print starts as that filename doesn't exist.

Another thing to mention is that the returning of the number of characters copied is not used anywhere in this project where strncpy is used.

The new strscpy is out of the scope of this PR so if you rather go with the new one than I would like to close this PR as this new strscpy is not what this PR was meant for, a safe, more lightweight, drop-in alternative to strncpy with some additional variants for future use-cases.

@bigtreetech
Let me know your thoughts on this.

[kisslorand]

@bigtreetech
Reverted "use strscpy" commit (6781bfd), rebased and fixed a bug in NULL-terminating the string in strscpy().