Add file size check at the beginning

I had a bug report because of a truncated vmcore file. The
error message just
was

    crash: read error: kernel virtual address:
ffff8107f3c3a6c0  type: "cpu_pda
    entry"

which is very unclear for a 'normal' user. This patch adds
checking of the file
size according to the ELF header at the beginning so that a
clear error message
can be printed.

Please consider adding the patch to crash.


Signed-off-by: Bernhard Walle <bwallesuse.de>

---
 defs.h    |    1 +
 netdump.c |   44
++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

--- a/defs.h
+++ b/defs.h
 -16,6
+16,7 
  * GNU General Public License for more details. 
  */
 
+#define _LARGEFILE64_SOURCE 1 /* stat64() */
 #ifndef GDB_COMMON
 
 #include <stdio.h>
--- a/netdump.c
+++ b/netdump.c
 -33,6
+33,47  static physaddr_t xen_kdump_p2m(physaddr
 #define ELFREAD  0
 
 #define MIN_PAGE_SIZE (4096)
+
+
+static int
+check_netdump_filesize(char *file)
+{
+	uint64_t max_file_offset = 0;
+	struct pt_load_segment *pls;
+	struct stat64 stat;
+	int i, ret;
+
+
+	/* find the maximum file offset */
+	for (i = 0; i < nd->num_pt_load_segments; i++) {
+		uint64_t end, size;
+
+		pls = &nd->pt_load_segments[i];
+
+		size = pls->phys_end - pls->phys_start;
+		end = pls->file_offset + size;
+
+		if (end > max_file_offset)
+			max_file_offset = end;
+	}
+
+	ret = stat64(file, &stat);
+	if (ret < 0) {
+		fprintf(stderr, "Cannot stat64 on %s: %sn",
file,
+				strerror(errno));
+		return FALSE;
+	}
+
+	if (max_file_offset > stat.st_size) {
+		fprintf(stderr, "File %s is too short:n"
+				"Must be %lld bytes but is only "
+				"%lld bytes long.n",
+				file, max_file_offset, stat.st_size);
+		return FALSE;
+	}
+
+	return TRUE;
+}
 	
 /*
  *  Determine whether a file is a netdump/diskdump/kdump
creation, 
 -267,6
+308,9  is_netdump(char *file, ulong source_quer
 	if (CRASHDEBUG(1))
 		netdump_memory_dump(fp);
 
+	if (!check_netdump_filesize(file))
+		return FALSE;
+
 	return nd->header_size;
 
 bailout:

--
Crash-utility mailing list
Crash-utilityredhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:
> I had a bug report because of a truncated vmcore file.
The error message just
> was
> 
>     crash: read error: kernel virtual address:
ffff8107f3c3a6c0  type: "cpu_pda
>     entry"
> 
> which is very unclear for a 'normal' user. This patch
adds checking of the file
> size according to the ELF header at the beginning so
that a clear error message
> can be printed.
> 
> Please consider adding the patch to crash.
> 

I'm hesitant about is_netdump() simply returning FALSE, and
therefore
essentially saying: "this isn't a netdump or kdump file
-- go check
whether it's some other dumpfile type".

And what if the dumpfile is short, but still usable?  We
sure
have seen a lot of real netdump files (i.e. not kdumps)
that
have been truncated, but still usable.

I understand the concern about the read error message, but
maybe
a better way to do it would be to just issue a WARNING
message?

Dave


> 
> Signed-off-by: Bernhard Walle <bwallesuse.de>
> 
> ---
>  defs.h    |    1 +
>  netdump.c |   44
++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> --- a/defs.h
> +++ b/defs.h
>  -16,6 +16,7 
>   * GNU General Public License for more details. 
>   */
>  
> +#define _LARGEFILE64_SOURCE 1 /* stat64() */
>  #ifndef GDB_COMMON
>  
>  #include <stdio.h>
> --- a/netdump.c
> +++ b/netdump.c
>  -33,6 +33,47  static physaddr_t xen_kdump_p2m(physaddr
>  #define ELFREAD  0
>  
>  #define MIN_PAGE_SIZE (4096)
> +
> +
> +static int
> +check_netdump_filesize(char *file)
> +{
> +	uint64_t max_file_offset = 0;
> +	struct pt_load_segment *pls;
> +	struct stat64 stat;
> +	int i, ret;
> +
> +
> +	/* find the maximum file offset */
> +	for (i = 0; i < nd->num_pt_load_segments; i++)
{
> +		uint64_t end, size;
> +
> +		pls = &nd->pt_load_segments[i];
> +
> +		size = pls->phys_end - pls->phys_start;
> +		end = pls->file_offset + size;
> +
> +		if (end > max_file_offset)
> +			max_file_offset = end;
> +	}
> +
> +	ret = stat64(file, &stat);
> +	if (ret < 0) {
> +		fprintf(stderr, "Cannot stat64 on %s:
%sn", file,
> +				strerror(errno));
> +		return FALSE;
> +	}
> +
> +	if (max_file_offset > stat.st_size) {
> +		fprintf(stderr, "File %s is too short:n"
> +				"Must be %lld bytes but is only "
> +				"%lld bytes long.n",
> +				file, max_file_offset, stat.st_size);
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
>  	
>  /*
>   *  Determine whether a file is a
netdump/diskdump/kdump creation, 
>  -267,6 +308,9  is_netdump(char *file, ulong source_quer
>  	if (CRASHDEBUG(1))
>  		netdump_memory_dump(fp);
>  
> +	if (!check_netdump_filesize(file))
> +		return FALSE;
> +
>  	return nd->header_size;
>  
>  bailout:
> 
> --
> Crash-utility mailing list
> Crash-utilityredhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility


--
Crash-utility mailing list
Crash-utilityredhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

* Dave Anderson <andersonredhat.com> [2007-09-25
15:59]:

> I understand the concern about the read error message,
but maybe
> a better way to do it would be to just issue a WARNING
message?

That's a valid point. So what about this one:


Signed-off-by: Bernhard Walle <bwallesuse.de>

---
 defs.h    |    1 +
 netdump.c |   43
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

--- a/defs.h
+++ b/defs.h
 -16,6
+16,7 
  * GNU General Public License for more details. 
  */
 
+#define _LARGEFILE64_SOURCE 1 /* stat64() */
 #ifndef GDB_COMMON
 
 #include <stdio.h>
--- a/netdump.c
+++ b/netdump.c
 -33,6
+33,47  static physaddr_t xen_kdump_p2m(physaddr
 #define ELFREAD  0
 
 #define MIN_PAGE_SIZE (4096)
+
+
+static int
+check_netdump_filesize(char *file)
+{
+	uint64_t max_file_offset = 0;
+	struct pt_load_segment *pls;
+	struct stat64 stat;
+	int i, ret;
+
+
+	/* find the maximum file offset */
+	for (i = 0; i < nd->num_pt_load_segments; i++) {
+		uint64_t end, size;
+
+		pls = &nd->pt_load_segments[i];
+
+		size = pls->phys_end - pls->phys_start;
+		end = pls->file_offset + size;
+
+		if (end > max_file_offset)
+			max_file_offset = end;
+	}
+
+	ret = stat64(file, &stat);
+	if (ret < 0) {
+		error(WARNING, "Cannot stat64 on %s: %sn. Checking
of file "
+				"size disabled", file, strerror(errno));
+		return FALSE;
+	}
+
+	if (max_file_offset > stat.st_size) {
+		error(WARNING, "File %s is too short:n"
+				"Must be %lld bytes but is only "
+				"%lld bytes long.n",
+				file, max_file_offset, stat.st_size);
+		return FALSE;
+	}
+
+	return TRUE;
+}
 	
 /*
  *  Determine whether a file is a netdump/diskdump/kdump
creation, 
 -267,6
+308,8  is_netdump(char *file, ulong source_quer
 	if (CRASHDEBUG(1))
 		netdump_memory_dump(fp);
 
+	check_netdump_filesize(file);
+
 	return nd->header_size;
 
 bailout:

--
Crash-utility mailing list
Crash-utilityredhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:
> * Dave Anderson <andersonredhat.com> [2007-09-25
15:59]:
> 
> 
>>I understand the concern about the read error
message, but maybe
>>a better way to do it would be to just issue a
WARNING message?
> 
> 
> That's a valid point. So what about this one:
> 

Bernhard,

I'm going to defer this one for now -- I'm trying to get a
release out today.

Anyway, the function wouldn't need to return anything, it
only works for netdumps or kdumps and not the other
dumpfile
types (not that I'm planning on implementing it on those
other types...), I'd really need to test it thoroughly,
etc...

Thanks,
   Dave


> 
> Signed-off-by: Bernhard Walle <bwallesuse.de>
> 
> ---
>  defs.h    |    1 +
>  netdump.c |   43
+++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> --- a/defs.h
> +++ b/defs.h
>  -16,6 +16,7 
>   * GNU General Public License for more details. 
>   */
>  
> +#define _LARGEFILE64_SOURCE 1 /* stat64() */
>  #ifndef GDB_COMMON
>  
>  #include <stdio.h>
> --- a/netdump.c
> +++ b/netdump.c
>  -33,6 +33,47  static physaddr_t xen_kdump_p2m(physaddr
>  #define ELFREAD  0
>  
>  #define MIN_PAGE_SIZE (4096)
> +
> +
> +static int
> +check_netdump_filesize(char *file)
> +{
> +	uint64_t max_file_offset = 0;
> +	struct pt_load_segment *pls;
> +	struct stat64 stat;
> +	int i, ret;
> +
> +
> +	/* find the maximum file offset */
> +	for (i = 0; i < nd->num_pt_load_segments; i++)
{
> +		uint64_t end, size;
> +
> +		pls = &nd->pt_load_segments[i];
> +
> +		size = pls->phys_end - pls->phys_start;
> +		end = pls->file_offset + size;
> +
> +		if (end > max_file_offset)
> +			max_file_offset = end;
> +	}
> +
> +	ret = stat64(file, &stat);
> +	if (ret < 0) {
> +		error(WARNING, "Cannot stat64 on %s: %sn.
Checking of file "
> +				"size disabled", file,
strerror(errno));
> +		return FALSE;
> +	}
> +
> +	if (max_file_offset > stat.st_size) {
> +		error(WARNING, "File %s is too short:n"
> +				"Must be %lld bytes but is only "
> +				"%lld bytes long.n",
> +				file, max_file_offset, stat.st_size);
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
>  	
>  /*
>   *  Determine whether a file is a
netdump/diskdump/kdump creation, 
>  -267,6 +308,8  is_netdump(char *file, ulong source_quer
>  	if (CRASHDEBUG(1))
>  		netdump_memory_dump(fp);
>  
> +	check_netdump_filesize(file);
> +
>  	return nd->header_size;
>  
>  bailout:


--
Crash-utility mailing list
Crash-utilityredhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

* Dave Anderson <andersonredhat.com> [2007-09-25
16:58]:
> Bernhard Walle wrote:
>> * Dave Anderson <andersonredhat.com> [2007-09-25
15:59]:
>>> I understand the concern about the read error
message, but maybe
>>> a better way to do it would be to just issue a
WARNING message?
>> That's a valid point. So what about this one:
>
> I'm going to defer this one for now -- I'm trying to
get a
> release out today.

Of course, nothing urgent.

> Anyway, the function wouldn't need to return anything,
it
> only works for netdumps or kdumps and not the other
dumpfile
> types (not that I'm planning on implementing it on
those
> other types...), I'd really need to test it thoroughly,
etc...

I just left the return value for debugging reasons. But it's
cleaner
to remove it, you're right:


Signed-off-by: Bernhard Walle <bwallesuse.de>

---
 defs.h    |    1 +
 netdump.c |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

--- a/defs.h
+++ b/defs.h
 -16,6
+16,7 
  * GNU General Public License for more details. 
  */
 
+#define _LARGEFILE64_SOURCE 1 /* stat64() */
 #ifndef GDB_COMMON
 
 #include <stdio.h>
--- a/netdump.c
+++ b/netdump.c
 -33,6
+33,45  static physaddr_t xen_kdump_p2m(physaddr
 #define ELFREAD  0
 
 #define MIN_PAGE_SIZE (4096)
+
+
+static void
+check_netdump_filesize(char *file)
+{
+	uint64_t max_file_offset = 0;
+	struct pt_load_segment *pls;
+	struct stat64 stat;
+	int i, ret;
+
+
+	/* find the maximum file offset */
+	for (i = 0; i < nd->num_pt_load_segments; i++) {
+		uint64_t end, size;
+
+		pls = &nd->pt_load_segments[i];
+
+		size = pls->phys_end - pls->phys_start;
+		end = pls->file_offset + size;
+
+		if (end > max_file_offset)
+			max_file_offset = end;
+	}
+
+	ret = stat64(file, &stat);
+	if (ret < 0) {
+		error(WARNING, "Cannot stat64 on %s: %sn. Checking
of file "
+				"size disabled", file, strerror(errno));
+		return;
+	}
+
+	if (max_file_offset > stat.st_size) {
+		error(WARNING, "File %s is too short:n"
+				"Must be %lld bytes but is only "
+				"%lld bytes long.n",
+				file, max_file_offset, stat.st_size);
+		return;
+	}
+}
 	
 /*
  *  Determine whether a file is a netdump/diskdump/kdump
creation, 
 -267,6
+306,8  is_netdump(char *file, ulong source_quer
 	if (CRASHDEBUG(1))
 		netdump_memory_dump(fp);
 
+	check_netdump_filesize(file);
+
 	return nd->header_size;
 
 bailout:

--
Crash-utility mailing list
Crash-utilityredhat.com
https://www.redhat.com/mailman/listinfo/crash-utility