leandrof/u-boot
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

checkpatch: Add warnings for using strn(cat|cpy)

Browse Source 
strn(cat|cpy) has a bad habit of not nul-terminating the destination,
resulting in constructions like

	strncpy(foo, bar, sizeof(foo) - 1);
	foo[sizeof(foo) - 1] = '\0';

However, it is very easy to forget about this behavior and accidentally
leave a string unterminated. This has shown up in some recent coverity
scans [1, 2] (including code recently touched by yours truly).

Fortunately, the guys at OpenBSD came up with strl(cat|cpy), which always
nul-terminate strings. These functions are already in U-Boot, so we should
encourage new code to use them instead of strn(cat|cpy).

[1] https://lists.denx.de/pipermail/u-boot/2021-March/442888.html
[2] https://lists.denx.de/pipermail/u-boot/2021-January/438073.html

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
This commit is contained in:
Sean Anderson 2021-03-11 00:15:45 -05:00 committed by Tom Rini
parent 69a7529831
commit d9c3050710
2 changed files with 19 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
scripts/checkpatch.pl
View File

@ -2365,6 +2365,12 @@ sub u_boot_line {
"Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible\n" . $herecurr);
}
# prefer strl(cpy|cat) over strn(cpy|cat)
if ($line =~ /\bstrn(cpy|cat)\s*\(/) {
WARN("STRL",
"strl$1 is preferred over strn$1 because it always produces a nul-terminated string\n" . $herecurr);
}
# use defconfig to manage CONFIG_CMD options
if ($line =~ /\+\s*#\s*(define|undef)\s+(CONFIG_CMD\w*)\b/) {
ERROR("DEFINE_CONFIG_CMD",

14
tools/patman/test_checkpatch.py
View File

@ -353,7 +353,7 @@ index 0000000..2234c87
Args:
pm: PatchMaker object to use
msg" Expected message (e.g. 'LIVETREE')
msg: Expected message (e.g. 'LIVETREE')
pmtype: Type of problem ('error', 'warning')
"""
result = pm.run_checkpatch()
@ -439,6 +439,18 @@ index 0000000..2234c87
self.check_struct('per_device_auto', '_priv', 'DEVICE_PRIV_AUTO')
self.check_struct('per_device_plat_auto', '_plat', 'DEVICE_PLAT_AUTO')
def check_strl(self, func):
"""Check one of the checks for strn(cpy|cat)"""
pm = PatchMaker()
pm.add_line('common/main.c', "strn%s(foo, bar, sizeof(foo));" % func)
self.checkSingleMessage(pm, "STRL",
"strl%s is preferred over strn%s because it always produces a nul-terminated string\n"
% (func, func))
def testStrl(self):
"""Check for uses of strn(cat|cpy)"""
self.check_strl("cat");
self.check_strl("cpy");
if __name__ == "__main__":
unittest.main()