Messages in this thread | | | From | jim.cromie@gmail ... | Date | Tue, 22 Sep 2020 08:23:48 -0600 | Subject | Re: [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.
| |