From: Doug Anderson <dianders@chromium.org>
To: Yuran Pereira <yuran.pereira@hotmail.com>
Cc: kgdb-bugreport@lists.sourceforge.net,
linux-trace-kernel@vger.kernel.org, jason.wessel@windriver.com,
daniel.thompson@linaro.org, rostedt@goodmis.org,
mhiramat@kernel.org, linux-kernel@vger.kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH 1/2] kdb: Replace the use of simple_strto with safer kstrto in kdb_main
Date: Tue, 5 Dec 2023 13:41:44 -0800 [thread overview]
Message-ID: <CAD=FV=VZ61XFb1Ks79BHr1jL1jwf_36wYXryy0ZXOz1xTQ9zOg@mail.gmail.com> (raw)
In-Reply-To: <GV1PR10MB6563E0F8DB2D335BD9CFE4D3E8B4A@GV1PR10MB6563.EURPRD10.PROD.OUTLOOK.COM>
Hi,
On Sun, Nov 19, 2023 at 4:07 PM Yuran Pereira <yuran.pereira@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.pereira@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();
next prev parent reply other threads:[~2023-12-05 21:42 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-20 0:04 [PATCH 0/2] Replace the use of simple_strtol/ul functions with kstrto Yuran Pereira
2023-11-20 0:07 ` [PATCH 1/2] kdb: Replace the use of simple_strto with safer kstrto in kdb_main Yuran Pereira
2023-12-05 21:41 ` Doug Anderson [this message]
2023-11-20 0:09 ` [PATCH 2/2] trace: kdb: Replace simple_strtoul with kstrtoul in kdb_ftdump Yuran Pereira
2023-12-05 21:41 ` Doug Anderson
2023-12-06 11:37 ` Daniel Thompson
2023-12-06 15:17 ` Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=VZ61XFb1Ks79BHr1jL1jwf_36wYXryy0ZXOz1xTQ9zOg@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=daniel.thompson@linaro.org \
--cc=jason.wessel@windriver.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=rostedt@goodmis.org \
--cc=yuran.pereira@hotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).