From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Thu, 11 Mar 2021 00:15:45 -0500 Subject: [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) In-Reply-To: <20210311051545.1886333-1-seanga2@gmail.com> References: <20210311051545.1886333-1-seanga2@gmail.com> Message-ID: <20210311051545.1886333-6-seanga2@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 --- Changes in v2: - Move check to u_boot_line scripts/checkpatch.pl | 6 ++++++ tools/patman/test_checkpatch.py | 14 +++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 755f4802a4..4e047586a6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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", diff --git a/tools/patman/test_checkpatch.py b/tools/patman/test_checkpatch.py index a4fec1d4c1..56af5265cc 100644 --- a/tools/patman/test_checkpatch.py +++ b/tools/patman/test_checkpatch.py @@ -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() -- 2.30.1