Add defensive NULL pointer guards across public APIs and extend tests

[filtede98]

This change adds defensive NULL pointer checks across public API functions to prevent potential null pointer dereferences when invalid or uninitialized pointers are supplied by caller code:

• mp_init, mp_init_size, mp_init_copy, mp_clear, mp_zero, mp_clamp, mp_grow, mp_shrink, mp_copy, mp_set, mp_exch
• Scalar/getter/setter macros in ommath_private.h (mp_set_u32, mp_set_i32, mp_get_u32, etc.)
• Arithmetic & division operations (mp_add, mp_sub, mp_mul, mp_div, mp_mod, mp_div_2d, mp_mod_2d, mp_mul_2d, mp_2expt, mp_lshd, mp_rshd, mp_signed_rsh)
• Comparison routines (mp_cmp, mp_cmp_mag, mp_cmp_d)
• Number-theoretic functions (mp_gcd, mp_lcm, mp_exptmod, mp_invmod, mp_sqrt, mp_is_square, mp_root_n, mp_log, mp_kronecker, mp_prime_is_prime, mp_prime_next_prime, mp_rand)
• Radix and binary serialization (mp_read_radix, mp_to_radix, mp_radix_size, mp_to_ubin, mp_from_ubin, mp_to_sbin, mp_from_sbin, mp_pack, mp_unpack, mp_pack_count, mp_fwrite, mp_fread)
• Added comprehensive unit tests in demo/test.c validating NULL pointer behavior across all covered routines.

[czurnieden]

Thank you very much, that was quite a lot of work done here!

At first: it is C, users shall be allowed to shot themselves in the foot.

But serious, not to check for NULL except where necessary and/or useful was an intentional decision.

#include <stdlib.h>
#include <tommath.h>

#define DEBUG_PRINT(ERROR_NUMBER, ERROR_GOTO)\
        do{\
           fprintf(stderr, "%s %d in %s: %s\n",\
           __FILE__, __LINE__, __FUNCTION__,\
            mp_error_to_string((ERROR_NUMBER)));\
           goto ERROR_GOTO;\
        }while(0)

static void mp_print(const char *s, const mp_int *a, int radix, FILE *stream)
{
   mp_err err;
   fputs(s, stream);
   err = mp_fwrite(a, radix, stream);
   if (err != MP_OKAY) {
      fprintf(stderr,"mp_fwrite in mp_print failed. error = %s\n", mp_error_to_string(err));
      /* An error from mp_fwrite is almost always fatal, no use to try saving it */
      exit(EXIT_FAILURE);
   }
   fputc('\n',stream);
}

int main(void){
   mp_err err = MP_OKAY;
   mp_int a, b, c, d;
#ifdef TRY_IT_WITH_A_SLEDGEHAMMER
   mp_int *k = NULL;
#endif
   if( (err = mp_init_multi(&a, &b, &c, NULL) ) != MP_OKAY)                                  DEBUG_PRINT(err, LTM_ERR);

   if( (err = mp_init(&d) ) != MP_OKAY)                                                      DEBUG_PRINT(err, LTM_ERR);
   mp_clear(&d);
   /* Will do what it is supposed to do, allocate memory for "d" because "d" exists */
   if( (err = mp_init(&d) ) != MP_OKAY)                                                      DEBUG_PRINT(err, LTM_ERR); 
   
   mp_set(&a, 123);
   mp_set(&b, 3210);
   /* Unused */
   mp_set(&c, 876);
   
   /* mp_clear sets b.dp to NULL, but the variable exists: no warning from the compiler */
   mp_clear(&b);
   if( (err = mp_div(&b, &a, NULL, &c) ) != MP_OKAY)                                         DEBUG_PRINT(err, LTM_ERR);
   /*
      Will print 0 (zero) instead of 12 (twelve).
      A wrong result without an error?
      Not exactly wrong. mp_clear free()'s the memory and sets the rest as if "d = 0"
      so mp_div computes 0/a.
    */
   mp_print("3210 % 123 = ", &c, 10, stdout);

   /* Double free() is allowed */
   mp_clear_multi(&a, &b, &c, &d, NULL);
   mp_clear_multi(&a, &b, &c, &d, NULL);
#ifdef TRY_IT_WITH_A_SLEDGEHAMMER
   if( (err = mp_init(k) ) != MP_OKAY) 
#endif
   exit(EXIT_SUCCESS);
LTM_ERR:
   fprintf(stderr, "An error occured %s \n",mp_error_to_string(err));
   mp_clear_multi(&a, &b, &c, &d, NULL);
   exit(EXIT_FAILURE);
}

And if we dust our off our sledgehammer?

The user needs to make two (honest) mistakes:

1. Wrongly declare k as a pointer.

User doesn't leave it uninitialized and sets it to NULL, as one does with freshly declared pointers.
User wrote mp_init(&k) and gets a compiler warning (clang -Weverything):

ltm_NULL_test.c:55:23: warning: incompatible pointer types passing 'mp_int **' to parameter of type 'mp_int *'; remove & [-Wincompatible-pointer-types]
   if( (err = mp_init(&k) ) != MP_OKAY) 
                      ^~
/usr/local/include/tommath.h:186:24: note: passing argument to parameter 'a' here
   mp_err mp_init(mp_int *a) MP_WUR;
                          ^
1 warning generated

So user does as told (because the compiler is always right, isn't it?) and

2. "corrects" to mp_init(k) by removing "&".

Gets a segfault as a result.
Is dumbfounded.
Runs debugger (here: gdb). Excerpt:

   Program received signal SIGSEGV, Segmentation fault.
   0x00007ffff7f7a697 in mp_init (a=a@entry=0x0) at mp_init.c:10
                                     ------>^^^

(Arrow added by me)

Caveat: different architectures may have different values for NULL, see C-FAQ at
http://c-faq.com/null/machexamp.html for some historical examples.
Be aware that some of those machines mentioned in that answer might still run.
If you come by one of those and it does compile LTM successfully: please throw us a note. Thanks!

Another point:
Some functions don't have the means to throw an error, e.g., mp_cmp, mp_count_bits and others. You would need to make some explicit error-values, which is possible (INT_MIN, INT_MAX, and UINT_MAX come to mind) but tedious, because those error values need to be checked by the function it uses, otherwise it would be useless.
Just setting the output to zero is misleading because a zero might be legit in some cases. As are MP_LT/MP_GT.

The error-handling in Libtommath is black and white: in case of an error say what kind of error, clean up and abort the function.

There are exceptions, of course, where the error gets used as a flag instead of an error (e.g., mp_log) to avoid redundant code, but the principle is still the same: the called function cleans up and returns the error.

This behavior makes backtracking a bit harder if the error happens deep inside a recursion. You might use sth. like the macro above for some kind of rudimentary backtracking, it can save some debugging time.

So it all boils down to the questions: how much hand-holding is necessary and am I willing to maintain all of the code involved in the aforementioned hand-holding?

[filtede98]

Totally fair and appreciate the detailed breakdown!

You're spot on about the non-mp_err functions (like mp_cmp, mp_count_bits) where shoehorning sentinels would be awkward and ambiguous.

The initial motivation was mostly defensive ergonomics for higher-level language bindings/FFI where catching a NULL early with MP_VAL is friendlier than an abrupt SIGSEGV. But I completely understand and respect LTM's design philosophy and the maintenance tradeoffs that come with defensive hand-holding in C.

If you prefer keeping LTM minimal and leaving pointer validation strictly to the caller, feel totally free to close this out. Alternatively, if you think it's useful to keep guards only on the key mp_err-returning entry points (like mp_init* / mp_read_* / mp_to_*), I'm happy to trim the patch down to just those.

Either way, thanks for taking the time to share the project's background and reasoning!