[Crash-utility] [External Mail][????] Re: [PATCH] netdump: bugfix for read elf header

[Mathias Krause]

Am 30.11.20 um 14:50 schrieb 赵乾利:
> Please found below strace,read size only 232,it's less then
> "SAFE_NETDUMP_ELF_HEADER_SIZE".
> [...]
> open("/var/tmp/ramdump_elf_viw34m", O_RDWR) = 5
> read(5,
> "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\4\0\267\0\1\0\0\0\0\0\0\0\0\0\0\0"...,
> 304) = 232

Thanks, so it's really a small file. Is it a valid one, though?

> [...]
> write(6, "/var/tmp/ramdump_elf_viw34m: ELF"...,
> 72/var/tmp/ramdump_elf_viw34m: ELF header read: No such file or directory
> ) = 72

And that message is just misleading, as it's a stale error...

> [...]
> 
>> Anyhow,this change introduces a regression to the code that's following,
> which
>> assumes the full 'size' was read, like the sanity checks for finding the
>> PT_NOTE program header. So you should either update 'size' so it mirrors
>> the actual bytes read or double-check that the kdump file you're trying
>> to analyze is actually a real one.
> 
> After apply the patch,issue is gone,so it's not kdump files problem.

Ok, thanks for clarifying.

> How about below patch set?
> 
> commit 8daecb704fa37a17e3c5294e35e593bfc1545016
> Author: Qianli Zhao <zhaoqianli at xiaomi.com>
> Date:   Mon Nov 30 17:17:32 2020 +0800
> 
>     netdump: bugfix for read elf header
>     
>     Without the patch,errors may occur in reading the ELF header,
>     causing the parsing to fail.header file size may smaller than
>     SAFE_NETDUMP_ELF_HEADER_SIZE
>     
>     Signed-off-by: Qianli Zhao <zhaoqianli at xiaomi.com>

The subject is too generic and the description not accurate enough. How
about something like this?:

    netdump: fix regression for tiny kdump files

    Commit f42db6a33f0e ("Support core files with "unusual" layout")
    increased the minimal file size from MIN_NETDUMP_ELF_HEADER_SIZE to
    SAFE_NETDUMP_ELF_HEADER_SIZE which lead to crash rejecting very
    small kdump files.

    Fix that by erroring out only if we get less than
    MIN_NETDUMP_ELF_HEADER_SIZE bytes.

> diff --git a/netdump.c b/netdump.c
> index c76d9dd..2e3e255 100644
> --- a/netdump.c
> +++ b/netdump.c
> @@ -142,7 +142,7 @@ is_netdump(char *file, ulong source_query)
>                 if (!read_flattened_format(fd, 0, eheader, size))
>                         goto bailout;
>         } else {
> -               if (read(fd, eheader, size) != size) {
> +               if ((size = read(fd, eheader, size)) <
> MIN_NETDUMP_ELF_HEADER_SIZE) {

As size is unsigned, this would lead to different errors in case the
read() fails, i.e. returns -1.

Can you please do something like this instead?:
- change type of 'size' to 'ssize_t'
- change the "else" block as follows:
  size = read(fd, eheader, size);
  if (size < MIN_NETDUMP_ELF_HEADER_SIZE) {
    sprintf(buf, "%s: ELF header read", file);
    if (size < 0)
      perror(buf);
    else
      fprintf(stderr, "%s: file too small!\n", buf);
    goto bailout;
  }

Thanks,
Mathias

>                         sprintf(buf, "%s: ELF header read", file);
>                         perror(buf);
>                         goto bailout;