[Crash-utility] [PATCH RFC 00/14] Minor code cleanups, round zero

[Oleksandr Natalenko]

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 at 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/e81db08bc69fb1a7a7e48f892c2038d992a71f6d
>>
>> 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