lkml.org 
[lkml]   [2020]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/2] dyndbg: dont panic over bad input
On Tue, Sep 22, 2020 at 2:08 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2020-09-21 13:04:32, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking). A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel. Instead pr_err and return -EINVAL.
> >
> > Reported-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> > lib/dynamic_debug.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> > } else {
> > for (end = buf; *end && !isspace(*end); end++)
> > ;
> > - BUG_ON(end == buf);
> > + if (end == buf) {
> > + pr_err("expected non-empty bareword");
> > + return -EINVAL;
>
> My understanding is that the BUG_ON() was there to catch eventual bugs
> in the algorithm.
>
> IMHO, it was never reachable in the original code:
>
> 1. The following lines will skip all spaces and bail out
> when we reached the trailing '\0':
>
> buf = skip_spaces(buf);
> if (!*buf)
> break; /* oh, it was trailing whitespace */
>
>
> 2. The following code will find the end of a quoted text:
>
> if (*buf == '"' || *buf == '\'') {
> int quote = *buf++;
> for (end = buf; *end && *end != quote; end++)
>
>
> 3. The else part will find end of non-quoted word:
>
> } else {
> for (end = buf; *end && !isspace(*end); end++)
>
> 4. The BUG_ON() will trigger when the above cycle reached the
> trailing '\0' or space.
>
> This will never happen because this situation was caught
> in the 1st step.
>
>
> Your patch triggered the BUG_ON() because it wanted to handle
> '=' as a special character and was incomplete.
>
> If you want to handle '=' special way. You need to do it the same way
> as with the space. You need to skip it in 1st step. And it must mark
> the end of the word in 4th step.
>
> But it will be more complicated. You must be able to handle
> mix of spaces and '='. I mean the following situations:
>
> word=word
> word =word
> word= word
> word = word
> word = = word /* failure ? */
>
>
> Back to the BUG_ON(). It might be changed to something like:
>
> pr_err("Internal error when parsing dynamic debug query\n");
> return -EINVAL;
>
>
> Best Regards,
> Petr


yes, the original patch was ill conceived

Im blaming Transient Acute Myopia,
where I couldnt see to the end of the line, where "=flags"
is a proper flag setting.

That "interferes" with using '=' to separate keyword=value.

As you outlined, its possible to implement something that handles both,
but I decided that handling keyword=value is just a convenience feature,
and isnt worth the added corner-cases and explanatory burden.

\
 
 \ /
  Last update: 2020-09-22 16:24    [W:0.065 / U:4.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site