[DTrace-devel] [PATCH] Do not allocate a 0-size strtab
Kris Van Hees
kris.van.hees at oracle.com
Wed Apr 8 14:58:36 PDT 2020
On Wed, Apr 08, 2020 at 02:49:22PM -0700, Eugene Loh wrote:
> On 04/08/2020 02:30 PM, Kris Van Hees wrote:
>
> > On Wed, Apr 08, 2020 at 01:53:58PM -0700, Eugene Loh wrote:
> >> On 04/08/2020 01:40 PM, Kris Van Hees wrote:
> >>> On Wed, Apr 08, 2020 at 01:35:55PM -0700, Eugene Loh wrote:
> >>>> On 04/08/2020 08:00 AM, Kris Van Hees wrote:
> >>>>> If there is no stirng data to be stored in a DIFO, we were allocating
> >>>> stirng -> string
> >>>>
> >>>> FWIW, if I look at the code, I typically see dtdo_strtab dereferenced
> >>>> without any guards against the scenario dtdo_strtab==NULL. I don't know
> >>>> the code well enough to know whether to worry. Should I?
> >>> If the memory allocation for dtdo_strtab failed, we bail by way of a fatal
> >>> error, which means nothing ever gets to use this. So yes, it is safe.
> >> But what I mean is that if the table has 0 size, we don't even try to
> >> allocate the table... we just set a NULL pointer, which is a different
> >> situation from an allocation failing and indicating an error. Again, I
> >> just don't know if this means there can be a problem. Maybe nothing to
> >> worry about: if a problem turns up, it'll make itself known!
> > Ah ok. Sorry, you confused me because you wrote "If the memory allocation for
> > dtdo_strtab failed" so I focused on the allocation failure case.
>
> ??? I believe those were your words. Anyhow, doesn't matter.
Woops, my apologies. I misread the level of quotation. My bad.
> > If we have no strings, dtdo_strlen will be 0, dtdo_strtab will be NULL, and
> > that is safe because when there are no strings, there is nothing in the DIFO
> > that will reference dtdo_strtab. If anything referenced a string it would be
> > an entry to either the variable table or one of the relocation tables. And
> > in that case those strings would have been added, and thus dtdo_strlen would
> > not be zero.
>
> Cool. Thanks for the explanation.
>
> >>>>> a 0-size memory block. There is no need for that - we can just assign
> >>>>> NULL to the dtdo_strtab member.
> >>>>>
> >>>>> In the case of the dt_link_stmt() code, we also happened to take the
> >>>>> dtdo_strlen value from the wrong DIFO (fdp instead of dp) causing a
> >>>>> 0-size strtab to be allocated into which we then try to write actual
> >>>>> data. That caused memory corruption.
> >>>>>
> >>>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> >>>>> ---
> >>>>> libdtrace/dt_cc.c | 14 +++++++++-----
> >>>>> libdtrace/dt_dlibs.c | 23 +++++++++++++----------
> >>>>> 2 files changed, 22 insertions(+), 15 deletions(-)
> >>>>>
> >>>>> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> >>>>> index db009461..fc678692 100644
> >>>>> --- a/libdtrace/dt_cc.c
> >>>>> +++ b/libdtrace/dt_cc.c
> >>>>> @@ -2381,11 +2381,15 @@ dt_link_stmt(dtrace_hdl_t *dtp, dtrace_prog_t *pgp, dtrace_stmtdesc_t *sdp,
> >>>>> * Write out the new string table.
> >>>>> */
> >>>>> dp->dtdo_strlen = dt_strtab_size(stab);
> >>>>> - dp->dtdo_strtab = dt_zalloc(dtp, fdp->dtdo_strlen);
> >>>>> - if (dp->dtdo_strtab == NULL)
> >>>>> - goto fail;
> >>>>> - dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> >>>>> - dp->dtdo_strtab);
> >>>>> + if (dp->dtdo_strlen > 0) {
> >>>>> + dp->dtdo_strtab = dt_zalloc(dtp, dp->dtdo_strlen);
> >>>>> + if (dp->dtdo_strtab == NULL)
> >>>>> + goto fail;
> >>>>> + dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> >>>>> + dp->dtdo_strtab);
> >>>>> + } else
> >>>>> + dp->dtdo_strtab = NULL;
> >>>>> +
> >>>>> dt_strtab_destroy(stab);
> >>>>>
> >>>>> /*
> >>>>> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> >>>>> index 58e01221..2b8fe21d 100644
> >>>>> --- a/libdtrace/dt_dlibs.c
> >>>>> +++ b/libdtrace/dt_dlibs.c
> >>>>> @@ -554,7 +554,6 @@ done:
> >>>>> for (fid = fun0; fid < symc; fid++) {
> >>>>> dtrace_difo_t *dp;
> >>>>> dof_relodesc_t *brp;
> >>>>> - size_t slen;
> >>>>>
> >>>>> fp = funcs[fid];
> >>>>> dp = fp->difo;
> >>>>> @@ -583,16 +582,20 @@ done:
> >>>>> rp++;
> >>>>> }
> >>>>>
> >>>>> - slen = dt_strtab_size(stab);
> >>>>> - dp->dtdo_strtab = dt_alloc(dtp, slen);
> >>>>> - dp->dtdo_strlen = slen;
> >>>>> - if (dp->dtdo_strtab == NULL) {
> >>>>> - fprintf(stderr, "Failed to alloc strtab\n");
> >>>>> - goto out;
> >>>>> - }
> >>>>> + dp->dtdo_strlen = dt_strtab_size(stab);
> >>>>> + if (dp->dtdo_strlen > 0) {
> >>>>> + dp->dtdo_strtab = dt_alloc(dtp, dp->dtdo_strlen);
> >>>>> + if (dp->dtdo_strtab == NULL) {
> >>>>> + fprintf(stderr, "Failed to alloc strtab\n");
> >>>>> + goto out;
> >>>>> + }
> >>>>> +
> >>>>> + dt_strtab_write(stab,
> >>>>> + (dt_strtab_write_f *)dt_strtab_copystr,
> >>>>> + dp->dtdo_strtab);
> >>>>> + } else
> >>>>> + dp->dtdo_strtab = NULL;
> >>>>>
> >>>>> - dt_strtab_write(stab, (dt_strtab_write_f *)dt_strtab_copystr,
> >>>>> - dp->dtdo_strtab);
> >>>>> dt_strtab_destroy(stab);
> >>>>> }
> >>>>>
> >>>> _______________________________________________
> >>>> DTrace-devel mailing list
> >>>> DTrace-devel at oss.oracle.com
> >>>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list