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@xxxxxxxxxx> 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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
