Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values

Joel Brobecker
2020-12-01T07:11:52Z

> >>> +void
> >>> +gdb_mpz::safe_export (gdb::array_view<gdb_byte> buf,
> >>> + int endian, size_t nails, bool unsigned_p) const
> >>
> >> Just wondering if you'll ever need to pass a non-zero value for nails.
> >> If not, I think you could simplify the code by just removing it.
> >
> > I don't know for sure. What about a scenario where we want to export
> > to a variable which is held in an area that's not a multiple number
> > of bytes?
> >
> > The code isn't dramatically complexified because of it, so perhaps
> > just keep it by default, and remove it later, if we find that
> > we never used it?
>
> Well, I find it more logical to do the other way around, add the
> complexity when you need it. Because I'm pretty sure you won't go set
> an alarm clock for one year from now, to remember to come back and check
> whether or not we are using that feature :). Also, I'm usually not keen
> on checking in things that aren't used, because we don't even know if
> they work / do what they are intended to do.
>
> If you know things are coming that are going to use it, I'm fine with
> leaving it in, but otherwise I just don't see the point. I'll let you
> decide.

That's no problem. I will remove it.

> Also, the name (bit) should be singular. So I think we should
> standardize on "%zu-bit".

Agreed.

> >>> + /* Do the export into a buffer allocated by GMP itself; that way,
> >>> + we can detect cases where BUF is not large enough to export
> >>> + our value, and thus avoid a buffer overlow. Normally, this should
> >>> + never happen, since we verified earlier that the buffer is large
> >>> + enough to accomodate our value, but doing this allows us to be
> >>> + extra safe with the export.
> >>> +
> >>> + After verification that the export behaved as expected, we will
> >>> + copy the data over to BUF. */
> >>> +
> >>> + size_t word_countp;
> >>> + gdb::unique_xmalloc_ptr<void> exported
> >>
> >> I'd prefer if we didn't use heap allocation unnecessarily. If we get
> >> things right, that isn't necessary. And if we can still assert after
> >> the call that we did indeed get it right, then we'll catch any case
> >> where we didn't.
> >
> > The problem with what you suggest is that, if we didn't do things right,
> > by the time we realize it, we'll have already gone through a buffer
> > overflow, with the associated random and uncontrolled damage. On Ubuntu,
> > we already know that the overflow causes an abort, so we wouldn't even
> > get to the point where we'd double-check. For me, this risk is bad enough
> > that it's well worth this (small) extra allocation. I don't think
> > this function is going to be called very frequently.
>
> The way I see it is that if we check after the fact, it would be a
> gdb_assert. So if we get it wrong, it results in either in a crash or a
> failed assertion. Both equally result in a bug report.
>
> But you're right, using the heap allocation makes it so we can't get it
> wrong, so that's an advantage. I just thought that we'd get it right
> for good now :).

Me too. If GMP had a routine that gave us that information prior to
calling mpz_export, we could use it and verify that we can confidently
call mpz_export without risking a buffer overflow. Right now, we have
a formula provided by the manual, but but given in a way that does not
instill great confidence ("a calculation like the following"). I'm also
concerned with future versions changing the formula and us not noticing.

> >>> @@ -258,26 +278,14 @@ template<typename T>
> >>> T
> >>> gdb_mpz::as_integer () const
> >>> {
> >>> - /* Initialize RESULT, because mpz_export only write the minimum
> >>> - number of bytes, including none if our value is zero! */
> >>> - T result = 0;
> >>> + T result;
> >>>
> >>> - gdb_mpz exported_val (val);
> >>> - if (std::is_signed<T>::value && mpz_cmp_ui (val, 0) < 0)
> >>> - {
> >>> - /* We want to use mpz_export to set the return value, but
> >>> - this function does not handle the sign. So give exported_val
> >>> - a value which is at the same time positive, and has the same
> >>> - bit representation as our negative value. */
> >>> - gdb_mpz neg_offset;
> >>> + this->safe_export (gdb::make_array_view ((gdb_byte *) &result,
> >>> + sizeof (result)),
> >>
> >> I'd suggest using
> >>
> >> {(gdb_byte *) &result, sizeof (result)}
> >>
> >> to make the array view, as suggested by Pedro earlier.
> >
> > I thought I had tried that and got an error, but I will try again.
>
> I tried it before making that suggestion and it worked, so if you have
> troubles let me know.

--
Joel