Re: [Crash-utility] [PATCH RFC 00/14] Minor code cleanups, round zero
Hi, Dave.
Thanks for your intensive review and accepting contribution.
Nevertheless, I'd appreciate if you explicitly comment on the following series:
lkcd_v8: address of an array always evaluates to true
lkcd_v7: address of an array always evaluates to true
lkcd_v5: address of an array always evaluates to true
lkcd_v2_v3: address of an array always evaluates to true
lkcd_v1: address of an array always evaluates to true
Let me explain why I think this should be fixed.
dump_header_t is a struct that contains the following member:
/* the panic string, if available */
char dh_panic_string[DUMP_PANIC_LEN];
This struct is statically initialized with zeroes:
static dump_header_t dump_header_v8 = { 0 };
and assigned to dh via pointer. Thus, in the following expression:
dh && dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
once dh is non-NULL, dh->dh_panic_string also has some address within
a struct, but its value may be NULL. IIRC, passing NULL to strstr is
an undefined behaviour, so, looks like that this statement should be
rewritten to check whether dh_panic_string is NULL:
dh && *dh->dh_panic_string && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or
dh && dh->dh_panic_string[0] && strstr(dh->dh_panic_string,
"\n") ? "" : "\n"
or even
dh && strlen(dh->dh_panic_string) && strstr(dh->dh_panic_string,
"\n")
? "" : "\n"
to avoid any confusion for potential code reviewer.
Am I correct?
Thanks.
On Mon, Oct 30, 2017 at 8:10 PM, Dave Anderson <anderson(a)redhat.com> wrote:
----- Original Message -----
> Hi, Dave, Daisuke.
>
> > OK, mystery solved.
> > …
> > Regardless, I fixed the patch to be based upon the kernel version, and queued
> > it for crash-7.1.2:
> >
> >
https://github.com/crash-utility/crash/commit/e81db08bc69fb1a7a7e48f892c2...
>
> Thanks for looking into it, now it makes more sense to me.
>
> > Probably it should just set addr_d = addr - amount, but that's what the code
effectively
> > does now (by mistake), so I'm just going to leave this alone.
>
> Would you, maybe, still prefer modifying it to addr_d = addr - amount anyway to avoid
both
> compiler and code reviewer confusion in the future please?
>
> > It's pretty much impossible to have a pml without its PAGE_PRESENT bit set,
> > so it's been a benign bug. But the fix is incorrect. They should be:
> >
> > if (!(*pml4 & _PAGE_PRESENT))
>
> Thanks for the explanation. Would you still prefer changing it to a proper
expression
> since original one is confusing (NOT is evaluated before &)?
>
> > Oleksandr, this looks good to me.
> > Thanks for your patch.
>
> Thank you for looking into it. Dave, would you prefer applying this
> patch directly now?
>
> Also, I'd still like to get a response regarding other patches in the
> series once you have some time.
Yeah, as I mentioned before, I am interested in fixing actual bugs that could
conceivably pose a real-life problem.
Dave
--
Best regards,
Oleksandr Natalenko (post-factum)
Software Maintenance Engineer