[PATCH] Fix potential NULL pointer dereference

[PATCH] Fix potential NULL pointer dereference

[Luis Machado]

This patch addresses a potential NULL pointer dereference when we try to
duplicate a string. The input pointer can be NULL and that may lead to
crashes. We just use a statically-allocated string to prevent bad things from
happening.

gdb/ChangeLog:
2016-10-24  Luis Machado  <lgustavo@codesourcery.com>

	* exec.c (exec_file_locate_attach): Prevent NULL pointer dereference
	when duplicating a string.
---
 gdb/ChangeLog |  5 +++++
 gdb/exec.c    | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 388cc1f..43175ff 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-10-24  Luis Machado  <lgustavo@codesourcery.com>
 
+	* exec.c (exec_file_locate_attach): Prevent NULL pointer dereference
+	when duplicating a string.
+
+2016-10-24  Luis Machado  <lgustavo@codesourcery.com>
+
 	* exec.c (exception_print_same): Fix string comparison to use
 	statically-allocated ones.
 
diff --git a/gdb/exec.c b/gdb/exec.c
index 67ecc63..5eeac44 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -221,13 +221,20 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
     }
   CATCH (err, RETURN_MASK_ERROR)
     {
+      const char *msg;
+
       if (err.message != NULL)
-	warning ("%s", err.message);
+	{
+	  warning ("%s", err.message);
+	  msg = err.message;
+	}
+      else
+	msg = "";
 
       prev_err = err;
 
       /* Save message so it doesn't get trashed by the catch below.  */
-      prev_err.message = xstrdup (err.message);
+      prev_err.message = xstrdup (msg);
     }
   END_CATCH
 
-- 
2.7.4

Re: [PATCH] Fix potential NULL pointer dereference

[Pedro Alves]

On 10/24/2016 10:52 PM, Luis Machado wrote:

> diff --git a/gdb/exec.c b/gdb/exec.c
> index 67ecc63..5eeac44 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -221,13 +221,20 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
>      }
>    CATCH (err, RETURN_MASK_ERROR)
>      {
> +      const char *msg;
> +
>        if (err.message != NULL)
> -	warning ("%s", err.message);
> +	{
> +	  warning ("%s", err.message);
> +	  msg = err.message;
> +	}
> +      else
> +	msg = "";
>  

This is overcomplicating a bit, IMO.

>        prev_err = err;
>  
>        /* Save message so it doesn't get trashed by the catch below.  */
> -      prev_err.message = xstrdup (err.message);
> +      prev_err.message = xstrdup (msg);


I think this one liner:

 +      if (err.message != NULL)
          prev_err.message = xstrdup (err.message);

would have the same effect, since exception_print_same
handles NULL ex.message?

Thanks,
Pedro Alves

Re: [PATCH] Fix potential NULL pointer dereference

[Luis Machado]

On 10/24/2016 05:26 PM, Pedro Alves wrote:
> On 10/24/2016 10:52 PM, Luis Machado wrote:
>
>> diff --git a/gdb/exec.c b/gdb/exec.c
>> index 67ecc63..5eeac44 100644
>> --- a/gdb/exec.c
>> +++ b/gdb/exec.c
>> @@ -221,13 +221,20 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
>>      }
>>    CATCH (err, RETURN_MASK_ERROR)
>>      {
>> +      const char *msg;
>> +
>>        if (err.message != NULL)
>> -	warning ("%s", err.message);
>> +	{
>> +	  warning ("%s", err.message);
>> +	  msg = err.message;
>> +	}
>> +      else
>> +	msg = "";
>>
>
> This is overcomplicating a bit, IMO.
>
>>        prev_err = err;
>>
>>        /* Save message so it doesn't get trashed by the catch below.  */
>> -      prev_err.message = xstrdup (err.message);
>> +      prev_err.message = xstrdup (msg);
>
>
> I think this one liner:
>
>  +      if (err.message != NULL)
>           prev_err.message = xstrdup (err.message);
>
> would have the same effect, since exception_print_same
> handles NULL ex.message?
>
> Thanks,
> Pedro Alves
>

Fine by me. I'll push that change then.

Re: [PATCH] Fix potential NULL pointer dereference

[Luis Machado]

On 10/24/2016 05:39 PM, Luis Machado wrote:
> On 10/24/2016 05:26 PM, Pedro Alves wrote:
>> On 10/24/2016 10:52 PM, Luis Machado wrote:
>>
>>> diff --git a/gdb/exec.c b/gdb/exec.c
>>> index 67ecc63..5eeac44 100644
>>> --- a/gdb/exec.c
>>> +++ b/gdb/exec.c
>>> @@ -221,13 +221,20 @@ exec_file_locate_attach (int pid, int
>>> defer_bp_reset, int from_tty)
>>>      }
>>>    CATCH (err, RETURN_MASK_ERROR)
>>>      {
>>> +      const char *msg;
>>> +
>>>        if (err.message != NULL)
>>> -    warning ("%s", err.message);
>>> +    {
>>> +      warning ("%s", err.message);
>>> +      msg = err.message;
>>> +    }
>>> +      else
>>> +    msg = "";
>>>
>>
>> This is overcomplicating a bit, IMO.
>>
>>>        prev_err = err;
>>>
>>>        /* Save message so it doesn't get trashed by the catch below.  */
>>> -      prev_err.message = xstrdup (err.message);
>>> +      prev_err.message = xstrdup (msg);
>>
>>
>> I think this one liner:
>>
>>  +      if (err.message != NULL)
>>           prev_err.message = xstrdup (err.message);
>>
>> would have the same effect, since exception_print_same
>> handles NULL ex.message?
>>
>> Thanks,
>> Pedro Alves
>>
>
> Fine by me. I'll push that change then.
>

Pushed as b5e1db87897cabfd9beb8b1bd49f7d965c0f2607 now.

Thanks.