Messages in this thread | | | Subject | Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs | From | Andy Shevchenko <> | Date | Wed, 07 Mar 2018 15:17:19 +0200 |
| |
On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
Thanks for the patch, my comments below.
> Attempting to print an object pointed to by a bad (usually ERR_PTR) > pointer > is a not so surprising error. Our code handles them inconsistently: > * two places print (null) if ptr<PAGE_SIZE > * one place prints (null) if abs(ptr)<PAGE_SIZE > * one place prints (null) only if !ptr > > Obviously, saying (null) for a small but non-0 value is misleading. > Thus, let's print: > * (null) for exactly 0 > * (err) if last page && abs(ptr)<=MAX_ERRNO > * (invalid) otherwise >
First of all, this patch is much more arguable than the other one in your small series.
"(invalid)" is invalid. Hint: there is a nice comment in the code why.
I'm in principle not putting explanation here to insist people to eventually _read and understand_ the code before doing anything.
Some comments below.
> +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : > "(invalid)")
It looks ugly.
> /** > * simple_strtoull - convert a string to an unsigned long long > * @cp: The start of the string > @@ -588,7 +590,7 @@ char *string(char *buf, char *end, const char *s, > struct printf_spec spec) > size_t lim = spec.precision; > > if ((unsigned long)s < PAGE_SIZE) > - s = "(null)"; > + s = BAD_PTR_STRING(s);
It doesn't make any sense before your patch 2.
> if ((unsigned long)dn < PAGE_SIZE) > - return string(buf, end, "(null)", spec); > + return string(buf, end, BAD_PTR_STRING(dn), spec);
It simple doesn't make sense.
The idea is to do it below, in the pointer. These certain lines are going to be removed by my patch.
> - return string(buf, end, "(null)", spec); > + return string(buf, end, BAD_PTR_STRING(ptr), spec);
Doesn't make sense before your patch 2.
> if ((unsigned long)save_str > (unsigned > long)-PAGE_SIZE > || (unsigned long)save_str < > PAGE_SIZE) > - save_str = "(null)"; > + save_str = BAD_PTR_STRING(save_str);
This is perhaps one valid change in such situation.
-- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
| |