[PATCH] mini-gmp: pass correct old_size to custom reallocate function

[Niels Möller]

minux <minux.ma at gmail.com> writes:

> Because we don't have separate docs for mini-gmp, and mini-gmp does
> provide mp_set_memory_functions, I think it's best if it can match
> gmp's behavior here to avoid confusions.

The differences are documented in mini-gmp/README (and your patch fixes
all but one of the documented differences).

> OK, patch updated. Now mini-gmp keeps track of old_size correctly and
> implements the behavior exactly as documented. And I have modified
> tests to ensure old_size is correct on realloc and free.
> I hope the added complexity is not too much.

Changes to mini-gmp.c look reasonable to me. Some detailed comments
below. 

> In fact, I think it makes the code of mpq_get_str slightly easier to
> read.

I'm not that familiar with the mpq functions. I hope Marco can comment.

> diff -r bcca14c8a090 mini-gmp/mini-gmp.c
> --- a/mini-gmp/mini-gmp.c	Thu Mar 05 00:43:26 2020 +0100
> +++ b/mini-gmp/mini-gmp.c	Sat Mar 07 13:04:26 2020 -0500
> @@ -352,7 +352,8 @@
>  }
>  
>  #define gmp_xalloc(size) ((*gmp_allocate_func)((size)))
> -#define gmp_free(p) ((*gmp_free_func) ((p), 0))
> +#define gmp_xfree(p, size) ((*gmp_free_func) ((p), (size)))
> +#define gmp_xrealloc(ptr, old_size, size) ((*gmp_reallocate_func)(ptr, old_size, size))
>  
>  static mp_ptr
>  gmp_xalloc_limbs (mp_size_t size)
> @@ -361,10 +362,17 @@
>  }
>  
>  static mp_ptr
> -gmp_xrealloc_limbs (mp_ptr old, mp_size_t size)
> +gmp_xrealloc_limbs (mp_ptr old, mp_size_t old_size, mp_size_t size)
>  {
> +  const size_t sl = sizeof (mp_limb_t);
>    assert (size > 0);
> -  return (mp_ptr) (*gmp_reallocate_func) (old, 0, size * sizeof (mp_limb_t));
> +  return (mp_ptr) gmp_xrealloc (old, old_size * sl, size * sl);
> +}

I'd prefer repeating sizeof (mp_limb_t), instead of the local sl
constant.

> @@ -956,11 +964,12 @@
>    mp_limb_t d, di;
>    mp_limb_t r;
>    mp_ptr tp = NULL;
> +  mp_size_t tps = 0;
>  
>    if (inv->shift > 0)
>      {
>        /* Shift, reusing qp area if possible. In-place shift if qp == np. */
> -      tp = qp ? qp : gmp_xalloc_limbs (nn);
> +      tp = qp ? qp : gmp_xalloc_limbs (tps = nn);

Please write the assignment tps = nn as a separate statement. Also, I
think name "tn" instead of "tps" would be more consistent with
surrounding code.

>        r = mpn_lshift (tp, np, nn, inv->shift);
>        np = tp;
>      }
> @@ -978,7 +987,7 @@
>  	qp[nn] = q;
>      }
>    if ((inv->shift > 0) && (tp != qp))
> -    gmp_free (tp);
> +    gmp_xfree_limbs (tp, tps);

Can the condition be simplified to just

  if (tps > 0)
    gmp_xfree_limbs (tp, tps);

?

> @@ -4143,7 +4152,7 @@
>  size_t
>  mpz_sizeinbase (const mpz_t u, int base)
>  {
> -  mp_size_t un;
> +  mp_size_t un, oun;
>    mp_srcptr up;
>    mp_ptr tp;
>    mp_bitcnt_t bits;
> @@ -4176,7 +4185,7 @@
>  	 10. */
>      }
>  
> -  tp = gmp_xalloc_limbs (un);
> +  tp = gmp_xalloc_limbs (oun = un);

I'd prefer 

  tn = un;

as a separate statement. And maybe clearer to leave un unchanged below, and
use tn as the loop variable?

>    mpn_copyi (tp, up, un);
>    mpn_div_qr_1_invert (&bi, base);
>  
> @@ -4189,7 +4198,7 @@
>      }
>    while (un > 0);
>  
> -  gmp_free (tp);
> +  gmp_xfree_limbs (tp, oun);
>    return ndigits;
>  }
>  
> @@ -4199,7 +4208,7 @@
>    unsigned bits;
>    const char *digits;
>    mp_size_t un;
> -  size_t i, sn;
> +  size_t i, sn, osn = 0;
>  
>    digits = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
>    if (base > 1)
> @@ -4220,15 +4229,15 @@
>  
>    sn = 1 + mpz_sizeinbase (u, base);
>    if (!sp)
> -    sp = (char *) gmp_xalloc (1 + sn);
> +    sp = (char *) gmp_xalloc (osn = 1 + sn);

I'd prefer

  if (!sp)
    {
      osn = 1 + sn;
      sp = (char *) gmp_xalloc (osn);
    }
  else
    osn = 0;

(and drop the zero initialization at the declaration above).

> +ret:
>    sp[sn] = '\0';
> +  if (osn) sp = gmp_xrealloc(sp, osn, sn + 1);
>    return sp;
>  }

Would it make sense to also check if realloc is needed?

  if (osn && osn != sn + 1)
    gmp_xrealloc(...);

> @@ -4305,7 +4316,7 @@
>        r->_mp_size = 0;
>        return -1;
>      }
> -  dp = (unsigned char *) gmp_xalloc (strlen (sp));
> +  dp = (unsigned char *) gmp_xalloc (dpn = strlen (sp));

  dpn = strlen (sp);

as a separate statement. And I'm not that fond of the name dpn. Maybe

  sn = strlen (sp);

and reorganize the loop as

  for (i = 0; i < sn; i++)
    {
      ...
      dp[i] = digit;
    }

> @@ -4382,7 +4393,7 @@
>    str = mpz_get_str (NULL, base, x);
>    len = strlen (str);
>    len = fwrite (str, 1, len, stream);
> -  gmp_free (str);
> +  gmp_xfree (str, len + 1);
>    return len;
>  }

This seems wrong in the case of i/o errors, where fwrite may return a
shorter item count.

> --- a/mini-gmp/tests/testutils.c	Thu Mar 05 00:43:26 2020 +0100
> +++ b/mini-gmp/tests/testutils.c	Sat Mar 07 13:04:26 2020 -0500
> @@ -84,31 +84,51 @@
>  static void *
>  tu_realloc (void *p, size_t old_size, size_t new_size)
>  {
> -  size_t *block = block_check (p);
> -  block = (size_t *) realloc (block, sizeof(size_t) + new_size + sizeof(block_end));
> +  size_t *old_block = block_check (p), *block;
> +  void *new;
> +  if (old_block[0] != old_size)
> +    {
> +      fprintf (stderr, "%s:%d: bad old_size: want %ld, got %ld.\n", __FILE__, __LINE__,
> +                 (long)old_block[0], (long)old_size);
> +      abort ();
> +    }
> +
> +  /* We do not use realloc here to test custom allocators where realloc
> +     requires old_size to be set to the length of data that needs to be
> +     copied from old to the newly allocated space. */

I think the above check of old_size should be enough for testing
mini-gmp. One can still use realloc for the actual allocation, no manual
memcpy.

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.