[OB] cli/cli-script.c, null ptr guard

[OB] cli/cli-script.c, null ptr guard

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: arg --]
[-- Type: application/octet-stream, Size: 1057 bytes --]

2007-06-28  Michael Snyder  <msnyder@access-company.com>

	* cli/cli-script.c (build_command_line): Add null pointer guard 
	(Coverity).
	
Index: cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.40
diff -p -r1.40 cli-script.c
*** cli/cli-script.c	27 Jan 2007 12:30:46 -0000	1.40
--- cli/cli-script.c	28 Jun 2007 21:47:25 -0000
*************** build_command_line (enum command_control
*** 95,101 ****
      = (struct command_line **) xmalloc (sizeof (struct command_line *)
  					* cmd->body_count);
    memset (cmd->body_list, 0, sizeof (struct command_line *) * cmd->body_count);
!   cmd->line = savestring (args, strlen (args));
    return cmd;
  }
  
--- 95,103 ----
      = (struct command_line **) xmalloc (sizeof (struct command_line *)
  					* cmd->body_count);
    memset (cmd->body_list, 0, sizeof (struct command_line *) * cmd->body_count);
!   if (args != NULL)
!     cmd->line = savestring (args, strlen (args));
! 
    return cmd;
  }
  

Re: [OB] cli/cli-script.c, null ptr guard

[Daniel Jacobowitz]

No, I don't think this is obvious.  What does it mean to have a null
string here and how can it happen?  I'm pretty sure it can't, and the
if check is just clutter.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] cli/cli-script.c, null ptr guard

[msnyder]

> No, I don't think this is obvious.  What does it mean to have a null
> string here and how can it happen?  I'm pretty sure it can't, and the
> if check is just clutter.

The reasoning is that, since we checked it for NULL in the
first statement of the function, we must believe that the
possibility exists for it to be NULL.

Re: [OB] cli/cli-script.c, null ptr guard

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 03:17:18PM -0700, msnyder@sonic.net wrote:
> > No, I don't think this is obvious.  What does it mean to have a null
> > string here and how can it happen?  I'm pretty sure it can't, and the
> > if check is just clutter.
> 
> The reasoning is that, since we checked it for NULL in the
> first statement of the function, we must believe that the
> possibility exists for it to be NULL.

Right.  So, is it a sensible check?  Or should it be removed, or
should the condition for the error be simplified?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] cli/cli-script.c, null ptr guard

[msnyder]

> On Thu, Jun 28, 2007 at 03:17:18PM -0700, msnyder@sonic.net wrote:
>> > No, I don't think this is obvious.  What does it mean to have a null
>> > string here and how can it happen?  I'm pretty sure it can't, and the
>> > if check is just clutter.
>>
>> The reasoning is that, since we checked it for NULL in the
>> first statement of the function, we must believe that the
>> possibility exists for it to be NULL.
>
> Right.  So, is it a sensible check?  Or should it be removed, or
> should the condition for the error be simplified?

Well, it either makes sense to check it for null, or it doesn't.
If the new test is redundant, so is the old one.  Whoever wrote
it in the first place seemed to think it was worth checking.

This is called from a number of places, but they are all local to the module.

Ultimately the argument comes from the command parser.
It's one of those typical (char *args, int from_tty) things.

I've never been sure whether those were guaranteed to be non-null, or not.

Re: [OB] cli/cli-script.c, null ptr guard

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 04:04:29PM -0700, Michael Snyder wrote:
> > On Thu, Jun 28, 2007 at 03:17:18PM -0700, msnyder@sonic.net wrote:
> >> > No, I don't think this is obvious.  What does it mean to have a null
> >> > string here and how can it happen?  I'm pretty sure it can't, and the
> >> > if check is just clutter.
> >>
> >> The reasoning is that, since we checked it for NULL in the
> >> first statement of the function, we must believe that the
> >> possibility exists for it to be NULL.
> >
> > Right.  So, is it a sensible check?  Or should it be removed, or
> > should the condition for the error be simplified?
> 
> Well, it either makes sense to check it for null, or it doesn't.
> If the new test is redundant, so is the old one.  Whoever wrote
> it in the first place seemed to think it was worth checking.
> 
> This is called from a number of places, but they are all local to the module.
> 
> Ultimately the argument comes from the command parser.
> It's one of those typical (char *args, int from_tty) things.

There's four calls to build_command_line.  Three are passed a freshly
incremented pointer, so it can never be NULL.  That's
if/while/commands.  The other one came from get_command_line.  Those
can be NULL - well, I'm not sure, but I think they can.  They're
always if/while.

So how about adding gdb_assert (args != NULL) after the error call,
like below?  If you follow where the result of this function goes,
if we actually set cmd->line = NULL we will crash.

-- 
Daniel Jacobowitz
CodeSourcery

2007-07-01  Daniel Jacobowitz  <dan@codesourcery.com>

	* cli/cli-script.c (build_command_line): Update NULL check.

Index: cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.41
diff -u -p -r1.41 cli-script.c
--- cli/cli-script.c	28 Jun 2007 21:48:54 -0000	1.41
+++ cli/cli-script.c	1 Jul 2007 15:55:26 -0000
@@ -85,6 +85,7 @@ build_command_line (enum command_control
 
   if (args == NULL && (type == if_control || type == while_control))
     error (_("if/while commands require arguments."));
+  gdb_assert (args != NULL);
 
   cmd = (struct command_line *) xmalloc (sizeof (struct command_line));
   cmd->next = NULL;
@@ -95,8 +96,7 @@ build_command_line (enum command_control
     = (struct command_line **) xmalloc (sizeof (struct command_line *)
 					* cmd->body_count);
   memset (cmd->body_list, 0, sizeof (struct command_line *) * cmd->body_count);
-  if (args != NULL)
-    cmd->line = savestring (args, strlen (args));
+  cmd->line = savestring (args, strlen (args));
 
   return cmd;
 }

Re: [OB] cli/cli-script.c, null ptr guard

[msnyder]

> On Thu, Jun 28, 2007 at 04:04:29PM -0700, Michael Snyder wrote:
>> > On Thu, Jun 28, 2007 at 03:17:18PM -0700, msnyder@sonic.net wrote:
>> >> > No, I don't think this is obvious.  What does it mean to have a
>> null
>> >> > string here and how can it happen?  I'm pretty sure it can't, and
>> the
>> >> > if check is just clutter.
>> >>
>> >> The reasoning is that, since we checked it for NULL in the
>> >> first statement of the function, we must believe that the
>> >> possibility exists for it to be NULL.
>> >
>> > Right.  So, is it a sensible check?  Or should it be removed, or
>> > should the condition for the error be simplified?
>>
>> Well, it either makes sense to check it for null, or it doesn't.
>> If the new test is redundant, so is the old one.  Whoever wrote
>> it in the first place seemed to think it was worth checking.
>>
>> This is called from a number of places, but they are all local to the
>> module.
>>
>> Ultimately the argument comes from the command parser.
>> It's one of those typical (char *args, int from_tty) things.
>
> There's four calls to build_command_line.  Three are passed a freshly
> incremented pointer, so it can never be NULL.  That's
> if/while/commands.  The other one came from get_command_line.  Those
> can be NULL - well, I'm not sure, but I think they can.  They're
> always if/while.
>
> So how about adding gdb_assert (args != NULL) after the error call,
> like below?  If you follow where the result of this function goes,
> if we actually set cmd->line = NULL we will crash.

Yes, OK, will commit as shown.

Re: [OB] cli/cli-script.c, null ptr guard

[msnyder]

> Yes, OK, will commit as shown.

Oh, sorry, rather -- "Approved, please commit".   ;-)

Re: [OB] cli/cli-script.c, null ptr guard

[Daniel Jacobowitz]

On Mon, Jul 02, 2007 at 06:14:27PM -0700, Michael Snyder wrote:
> 
> > Yes, OK, will commit as shown.
> 
> Oh, sorry, rather -- "Approved, please commit".   ;-)

Done :-)

-- 
Daniel Jacobowitz
CodeSourcery