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

[Joel Brobecker <brobecker@adacore.com>]

> >>> +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