Fix crashes in bz24364

Message ID CALoOobOmJyDjMSDJ0EX3SGLago7vUKHbZw2KnsAo6VpjrTBh3g@mail.gmail.com
State New, archived
Headers

Commit Message

Terekhov, Mikhail via Gdb-patches June 17, 2019, 2:41 p.m. UTC
  This is somewhat on the obvious side.

Fix all crashes from bz24364.

gdb/ChangeLog

        PR gdb/24364
        * gdb/dtrace-probe.c (dtrace_static_probe_ops::get_probe): Don't
        call dtrace_process_dof with NULL dof.
  

Comments

Tom Tromey June 17, 2019, 5:38 p.m. UTC | #1
>>>>> "Paul" == Paul Pluzhnikov via gdb-patches <gdb-patches@sourceware.org> writes:

Paul> This is somewhat on the obvious side.
Paul> Fix all crashes from bz24364.

Paul> gdb/ChangeLog

Paul>         PR gdb/24364
Paul>         * gdb/dtrace-probe.c (dtrace_static_probe_ops::get_probe): Don't
Paul>         call dtrace_process_dof with NULL dof.

Thank you for the patch.  This is ok.

Tom
  
Simon Marchi June 17, 2019, 7:31 p.m. UTC | #2
On 2019-06-17 13:38, Tom Tromey wrote:
>>>>>> "Paul" == Paul Pluzhnikov via gdb-patches 
>>>>>> <gdb-patches@sourceware.org> writes:
> 
> Paul> This is somewhat on the obvious side.
> Paul> Fix all crashes from bz24364.
> 
> Paul> gdb/ChangeLog
> 
> Paul>         PR gdb/24364
> Paul>         * gdb/dtrace-probe.c 
> (dtrace_static_probe_ops::get_probe): Don't
> Paul>         call dtrace_process_dof with NULL dof.
> 
> Thank you for the patch.  This is ok.
> 
> Tom

Could you please just change the commit title to state what is changed 
concretely?  Also, put in the commit message at least a small example of 
how to trigger the bug.

Simon
  
Terekhov, Mikhail via Gdb-patches June 17, 2019, 8:56 p.m. UTC | #3
Simon,

On Mon, Jun 17, 2019 at 12:31 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2019-06-17 13:38, Tom Tromey wrote:
> >>>>>> "Paul" == Paul Pluzhnikov via gdb-patches
> >>>>>> <gdb-patches@sourceware.org> writes:
> >
> > Paul> This is somewhat on the obvious side.
> > Paul> Fix all crashes from bz24364.
> >
> > Paul> gdb/ChangeLog
> >
> > Paul>         PR gdb/24364
> > Paul>         * gdb/dtrace-probe.c
> > (dtrace_static_probe_ops::get_probe): Don't
> > Paul>         call dtrace_process_dof with NULL dof.
> >
> > Thank you for the patch.  This is ok.
> >
> > Tom
>
> Could you please just change the commit title to state what is changed
> concretely?

I've already committed this patch, but I'd like to understand your
request for the future.

What changed concretely is that we don't call dtrace_process_dof()
with NULL dof anymore. I am not sure how to express this clearer.

>  Also, put in the commit message at least a small example of
> how to trigger the bug.

To trigger the bug, run gdb on any of the examples attached to bugzilla 24364.
They are all artificial (generated by fuzzing).

Cheers,
  
Simon Marchi June 17, 2019, 9:08 p.m. UTC | #4
On 2019-06-17 4:56 p.m., Paul Pluzhnikov wrote:
> I've already committed this patch, but I'd like to understand your
> request for the future.
> 
> What changed concretely is that we don't call dtrace_process_dof()
> with NULL dof anymore. I am not sure how to express this clearer.

Ah, my bad.  It's just that usually, the email subject is also the commit title,
so I thought that "Fix crashes in bz24364" would end up as the commit message.  I
see that it is "PR gdb/24364: Don't call dtrace_process_dof with NULL dof." in
reality, this is fine.

>>  Also, put in the commit message at least a small example of
>> how to trigger the bug.
> 
> To trigger the bug, run gdb on any of the examples attached to bugzilla 24364.
> They are all artificial (generated by fuzzing).

When possible I find it useful to give a summary of the bug in the commit message
(including the steps that lead to the crash).  It makes it much easier to find the
information when doing archaeology than having to refer to the bugzilla (especially
that the bugzilla may not be there forever).  Although with a fuzzed binary it's not
as obvious, since you can't really describe the binary (though we could add a
test with it).

Simon
  

Patch

diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 52973784e9..2a2eae1f70 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -856,13 +856,14 @@  dtrace_static_probe_ops::get_probes
 
 	  /* Read the contents of the DOF section and then process it to
 	     extract the information of any probe defined into it.  */
-	  if (!bfd_malloc_and_get_section (abfd, sect, &dof))
+	  if (bfd_malloc_and_get_section (abfd, sect, &dof) && dof != NULL)
+	    dtrace_process_dof (sect, objfile, probesp,
+			        (struct dtrace_dof_hdr *) dof);
+         else
 	    complaint (_("could not obtain the contents of"
 			 "section '%s' in objfile `%s'."),
 		       sect->name, abfd->filename);
-      
-	  dtrace_process_dof (sect, objfile, probesp,
-			      (struct dtrace_dof_hdr *) dof);
+
 	  xfree (dof);
 	}
     }