Hi Dave,

Crash seems to assume that the "task_state_array" is NULL terminated. This is
not the case:

static const char *task_state_array[] = {
"R (running)", /* 0 */
"S (sleeping)", /* 1 */
...
"X (dead)" /* 32 */
};

I have a dump where this leads to a crash crash.

I think, when reading the array, we should use the array size as
loop exit criteria instead of checking for NULL termination.

Michael
---
diff -Naurp crash-5.0.6/task.c crash-5.0.6-task_state_array-fix//task.c
--- crash-5.0.6/task.c 2010-07-19 21:21:33.000000000 +0200
+++ crash-5.0.6-task_state_array-fix//task.c 2010-08-27 15:22:16.000000000 +0200
@@ -4296,6 +4296,7 @@ initialize_task_state(void)
ulong bitpos;
ulong str, task_state_array;
char buf[BUFSIZE];
+ int i;

if (!symbol_exists("task_state_array") ||
!readmem(task_state_array = symbol_value("task_state_array"),
@@ -4313,7 +4314,7 @@ old_defaults:
}

bitpos = 0;
- while (str) {
+ for (i = 0; i < get_array_length("task_state_array", NULL, 0); i++) {
if (!read_string(str, buf, BUFSIZE-1))
break;


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

----- "Michael Holzheu" <holzheu@linux.vnet.ibm.com> wrote:

> Hi Dave,
>
> Crash seems to assume that the "task_state_array" is NULL terminated.
> This is
> not the case:
>
> static const char *task_state_array[] = {
> "R (running)", /* 0 */
> "S (sleeping)", /* 1 */
> ...
> "X (dead)" /* 32 */
> };
>
> I have a dump where this leads to a crash crash.
>
> I think, when reading the array, we should use the array size as
> loop exit criteria instead of checking for NULL termination.

Agreed -- I'll just change your patch to just call get_array_length()
one time, and stash the result for use by the loop.

Qeued for the next release.

Thanks Mike,
Dave


>
> Michael
> ---
> diff -Naurp crash-5.0.6/task.c
> crash-5.0.6-task_state_array-fix//task.c
> --- crash-5.0.6/task.c 2010-07-19 21:21:33.000000000 +0200
> +++ crash-5.0.6-task_state_array-fix//task.c 2010-08-27
> 15:22:16.000000000 +0200
> @@ -4296,6 +4296,7 @@ initialize_task_state(void)
> ulong bitpos;
> ulong str, task_state_array;
> char buf[BUFSIZE];
> + int i;
>
> if (!symbol_exists("task_state_array") ||
> !readmem(task_state_array = symbol_value("task_state_array"),
> @@ -4313,7 +4314,7 @@ old_defaults:
> }
>
> bitpos = 0;
> - while (str) {
> + for (i = 0; i < get_array_length("task_state_array", NULL, 0); i++)
> {
> if (!read_string(str, buf, BUFSIZE-1))
> break;

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

On Fri, 2010-08-27 at 09:48 -0400, Dave Anderson wrote:
> ----- "Michael Holzheu" <holzheu@linux.vnet.ibm.com> wrote:
> > I think, when reading the array, we should use the array size as
> > loop exit criteria instead of checking for NULL termination.
>
> Agreed -- I'll just change your patch to just call get_array_length()
> one time, and stash the result for use by the loop.

Ok, this makes sense.

Thanks!

Michael

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