Hi,
On Sun, Nov 19, 2023 at 4:07 PM Yuran Pereira <yuran.pere...@hotmail.com> wrote:
>
> The simple_str* family of functions perform no error checking in
> scenarios where the input value overflows the intended output variable.
> This results in these functions successfully returning even when the
> output does not match the input string.
>
> Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(),
> simple_strtoul(), and simple_strtoull() functions explicitly ignore
> overflows, which may lead to unexpected results in callers."
> Hence, the use of those functions is discouraged.
>
> This patch replaces all uses of the simple_strto* series of functions
> with their safer kstrto* alternatives.
>
> Side effects of this patch:
> - Every string to long or long long conversion using kstrto* is now
> checked for failure.
> - kstrto* errors are handled with appropriate `KDB_BADINT` wherever
> applicable.
> - A good side effect is that we end up saving a few lines of code
> since unlike in simple_strto* functions, kstrto functions do not
> need an additional "end pointer" variable, and the return values
> of the latter can be directly checked in an "if" statement without
> the need to define additional `ret` or `err` variables.
> This, of course, results in cleaner, yet still easy to understand
> code.
>
> [1]
> https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull
>
> Signed-off-by: Yuran Pereira <yuran.pere...@hotmail.com>
> ---
> kernel/debug/kdb/kdb_main.c | 70 +++++++++++--------------------------
> 1 file changed, 21 insertions(+), 49 deletions(-)
Sorry for taking so long to review this--it arrived in my inbox at a
bad time. A few minor nits below that I think should be fixed before
landing but overall I think it's a nice cleanup. Thanks!
> @@ -412,42 +412,21 @@ static void kdb_printenv(void)
> */
> int kdbgetularg(const char *arg, unsigned long *value)
> {
> - char *endp;
> - unsigned long val;
> -
> - val = simple_strtoul(arg, &endp, 0);
> -
> - if (endp == arg) {
> - /*
> - * Also try base 16, for us folks too lazy to type the
> - * leading 0x...
> - */
> - val = simple_strtoul(arg, &endp, 16);
> - if (endp == arg)
> + /*
> + * If the first fails, also try base 16, for us
> + * folks too lazy to type the leading 0x...
> + */
> + if (kstrtoul(arg, 0, value))
> + if (kstrtoul(arg, 16, value))
Not new to your patch, but the above seems like a terrible idea to me.
What that means is that:
kdbgetularg("18", &value) => value is 18
kdbgetularg("19", &value) => value is 19
kdbgetularg("1a", &value) => value is 26
Bleh! If someone wants hex then they should put the 0x first.
I'd suggest a followup patch that removes the fallback for the lazy
folks. Here and in the next function...
> @@ -2095,15 +2074,11 @@ static int kdb_dmesg(int argc, const char **argv)
> if (argc > 2)
> return KDB_ARGCOUNT;
> if (argc) {
> - char *cp;
> - lines = simple_strtol(argv[1], &cp, 0);
> - if (*cp)
> + if (kstrtoint(argv[1], 0, &lines))
> lines = 0;
> - if (argc > 1) {
> - adjust = simple_strtoul(argv[2], &cp, 0);
> - if (*cp || adjust < 0)
> + if (argc > 1)
> + if (kstrtouint(argv[2], 0, &adjust) || adjust < 0)
My gut reaction is that some sort of build bot is going to come and
yell at you about the above line. Even if it doesn't, it's a bit
confusing. You're passing a pointer to an int into a function that
expects a pointer to an unsigned int. Most things don't really care
about signed/unsigned, but I could swear that some compilers get mad
when you start working with pointers to those types...
In any case, I think everything would work fine if you just change it
to kstrtoint(), right? I guess the other option would be to change the
variable to unsigned, but I guess that doesn't make sense since it's a
modifier to "lines" which is an int.
Side note: I didn't even know about the "adjust" argument, since it's
not in the help text in the command table below. I guess that could be
fixed in a separate patch.
nit: IMO if you have nested "if" statements then the outer one should
have braces. AKA:
if (a) {
if (b)
blah();
}
instead of:
if (a)
if (b)
blah();
...or you could do better and just change it to:
if (a && b)
blah();
