Hi,
Right now, TF-A coding guidelines advocate the use of strlcpy() over strcpy()/strncpy() (see the banned libc functions [1]) for the following reasons:
- strlcpy() always null-terminates the destination string, even if it gets truncated. This is safer and less error-prone than leaving it unterminated like strncpy() does in this case.
- With strlcpy(), it is possible to detect whether the string was truncated. This is useful in some use cases.
- strlcpy() writes a single null-terminating byte into the destination string, whereas strncpy() wastes time writing multiple null-terminating bytes until it fills the destination string. Thus, strlcpy() is more efficient in the case where the source string is shorter than the destination one.
There exists another string copy variant: strscpy(). Just like strlcpy(), it is not part of the C standard but most libc implementations provide it. strscpy() has the following advantages over strlcpy():
- strscpy() only reads the specified number of bytes from the source string, whereas strlcpy() reads all of its bytes until it finds a null-terminating byte (this is because it needs to return this information).
This opens up a potential security risk if the source string is untrusted and controlled by an attacker, as we might end up reading a lot of memory if proper safeguards are not put in place on the source string prior to calling strlcpy().
Also it causes a performance penalty for reading "useless" bytes from the source string.
- Checking for truncation is easier and less error-prone with strscpy() : strscpy() returns an error code (-E2BIG) in case of truncation, whereas strlcpy() requires comparing the return value with the destination string size.
For these reasons, it seems to me we should prefer strscpy() over strlcpy() in TF-A code base. Any thoughts?
Note that the Linux kernel coding style already goes in that direction. The coding style verification script (checkpatch.pl), which TF-A reuses, will print the following warning message if strlcpy() is used:
WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmkn...
This got flagged by the OpenCI into some TF-A code review already [2].
If we decide that strlcpy() is fine for TF-A codebase in the end, we'll at least need to find a way to silence this checkpatch.pl warning. AFAICS there is already a switch to this effect so it should just be a matter of adding '--ignore STRLCPY' inside .checkpatch.conf file.
Best regards, Sandrine
[1] https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...) [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/25551