[PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Hui Zhu]

Hi,

I found this bug when I try to fix 15180.
In function build_target_command_list:
When it try to release the loc->cmd_bytecode in the first loop, it try
to release loc->cond_bytecode:
  /* If anything failed, then we're not doing target-side commands,
     and so clean up.  */
  if (null_command_or_parse_error)
    {
      ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
	{
	  loc = (*loc2p);
	  if (is_breakpoint (loc->owner)
	      && loc->pspace->num == bl->pspace->num)
	    {
	      /* Only go as far as the first NULL bytecode is
		 located.  */
	      if (!loc->cond_bytecode)
		return;

	      free_agent_expr (loc->cond_bytecode);
	      loc->cond_bytecode = NULL;
	    }
	}
    }

I think it will crash GDB something.  So I suggest fix it before 7.6 release.

Thanks,
Hui

2013-04-24  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (build_target_command_list): Change loc->cond_bytecode
	to loc->cmd_bytecode.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2343,11 +2343,11 @@ build_target_command_list (struct bp_loc
 	    {
 	      /* Only go as far as the first NULL bytecode is
 		 located.  */
-	      if (!loc->cond_bytecode)
+	      if (!loc->cmd_bytecode)
 		return;

-	      free_agent_expr (loc->cond_bytecode);
-	      loc->cond_bytecode = NULL;
+	      free_agent_expr (loc->cmd_bytecode);
+	      loc->cmd_bytecode = NULL;
 	    }
 	}
     }

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> 2013-04-24  Hui Zhu  <hui@codesourcery.com>

Hui> 	* breakpoint.c (build_target_command_list): Change loc->cond_bytecode
Hui> 	to loc->cmd_bytecode.

Is it possible to make a test case for this?

The patch looks good.

I am curious about this code in build_target_command_list:

	      aexpr = parse_cmd_to_aexpr (bl->address,
					  loc->owner->extra_string);
	      loc->cmd_bytecode = aexpr;

	      if (!aexpr)
		continue;

The "continue" seems to mean that null_command_or_parse_error will not
be set in the "parse error" case.

Also, parse_cmd_to_aexpr calls 'error' in a few spots but then in
another spot is careful not to.  This seems somewhat odd.

Tom

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Hui Zhu]

On Wed, Apr 24, 2013 at 10:30 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> 2013-04-24  Hui Zhu  <hui@codesourcery.com>
>
> Hui>    * breakpoint.c (build_target_command_list): Change loc->cond_bytecode
> Hui>    to loc->cmd_bytecode.
>
> Is it possible to make a test case for this?

I tried but looks free conditions cannot crash GDB.  :(

>
> The patch looks good.
>
> I am curious about this code in build_target_command_list:
>
>               aexpr = parse_cmd_to_aexpr (bl->address,
>                                           loc->owner->extra_string);
>               loc->cmd_bytecode = aexpr;
>
>               if (!aexpr)
>                 continue;
>
> The "continue" seems to mean that null_command_or_parse_error will not
> be set in the "parse error" case.

I think it is right, because even if one of breakpoint loc doesn't
have commands.  GDB still need send other commands to target.

>
> Also, parse_cmd_to_aexpr calls 'error' in a few spots but then in
> another spot is careful not to.  This seems somewhat odd.

This function looks have something still not right.  I am just working
on 15180 to make it better.

On 04/24/2013 09:29 PM, Hui Zhu wrote:
-             if (!loc->cond_bytecode)
+             if (!loc->cmd_bytecode)

Nit:          "if (loc->cmd_bytecode == NULL)" is better.

Fixed.

Thanks,
Hui

>
> Tom

2013-04-24  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (build_target_command_list): Change loc->cond_bytecode
	to loc->cmd_bytecode.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2343,11 +2343,11 @@ build_target_command_list (struct bp_loc
 	    {
 	      /* Only go as far as the first NULL bytecode is
 		 located.  */
-	      if (!loc->cond_bytecode)
+	      if (loc->cmd_bytecode == NULL)
 		return;

-	      free_agent_expr (loc->cond_bytecode);
-	      loc->cond_bytecode = NULL;
+	      free_agent_expr (loc->cmd_bytecode);
+	      loc->cmd_bytecode = NULL;
 	    }
 	}
     }

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> I tried but looks free conditions cannot crash GDB.  :(

I was wondering if there could be a non-crashing reproducer.
It seems a little tricky.

I think this particular patch is ok without a test though.
Please check it in.

>> The "continue" seems to mean that null_command_or_parse_error will not
>> be set in the "parse error" case.

Hui> I think it is right, because even if one of breakpoint loc doesn't
Hui> have commands.  GDB still need send other commands to target.

I think that would be an argument for removing the "break" -- but not
necessarily for keeping the "continue".

The "return" in the null_command_or_parse_error case also seems weird to
me, but I didn't think hard about it.

Tom

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Hui Zhu]

On Wed, Apr 24, 2013 at 11:35 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> I tried but looks free conditions cannot crash GDB.  :(
>
> I was wondering if there could be a non-crashing reproducer.
> It seems a little tricky.
>
> I think this particular patch is ok without a test though.
> Please check it in.

Checked in http://www.sourceware.org/ml/gdb-cvs/2013-04/msg00230.html
Can I check in it to 7.6 branch?

>
>>> The "continue" seems to mean that null_command_or_parse_error will not
>>> be set in the "parse error" case.
>
> Hui> I think it is right, because even if one of breakpoint loc doesn't
> Hui> have commands.  GDB still need send other commands to target.
>
> I think that would be an argument for removing the "break" -- but not
> necessarily for keeping the "continue".
>
> The "return" in the null_command_or_parse_error case also seems weird to
> me, but I didn't think hard about it.

Because current function is hard to handle two or more breakpoints in
same address with different commands and conditions.  So maybe I will
rewrite this function.
Please keep help me with that when I post patch for them.

Thanks,
Hui

>
> Tom

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Joel Brobecker]

> Checked in http://www.sourceware.org/ml/gdb-cvs/2013-04/msg00230.html
> Can I check in it to 7.6 branch?

OK for 7.6.

-- 
Joel

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Hui Zhu]

On Thu, Apr 25, 2013 at 2:53 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Checked in http://www.sourceware.org/ml/gdb-cvs/2013-04/msg00230.html
>> Can I check in it to 7.6 branch?
>
> OK for 7.6.

Checked in http://www.sourceware.org/ml/gdb-cvs/2013-04/msg00238.html

Thanks,
Hui

>
> --
> Joel

Re: [PATCH/7.6] Fix wrong release (maybe crash GDB) in build_target_command_list

[Yao Qi]

On 04/24/2013 09:29 PM, Hui Zhu wrote:
> -	      if (!loc->cond_bytecode)
> +	      if (!loc->cmd_bytecode)

Nit:          "if (loc->cmd_bytecode == NULL)" is better.

-- 
Yao (齐尧)