[9front] libmp and error handling

[9front] libmp and error handling

[covertusername967]

Hello,

I was working with libmp today trying to figure out error handling.
mp(2) says the following:

          Routines that return an mpint will allocate the mpint if the
          result parameter is nil.  This includes strtomp, itomp,
          uitomp, btomp, and dtomp. These functions, in addition to
          mpnew and mpcopy, will return nil if the allocation fails.

However, reading the source code reveals something entirely different:
while the routines do allocate a new mpint if needed, if allocation
fails they don't return nil and instead outright call sysfatal.  I
then checked the rest of the code in the system that uses libmp and
all of it relies on the undocumented behaviour, assuming that returned
pointers are valid.

There's both an easy way and a hard way to fix this.  The easy way
would be to document the sysfatal call in the manpage and mention that
sysfatal should be overridden in order to change the error handling.
The hard way would be to remove the sysfatal calls in libmp, then
change the rest of the system so that it catches the errors.

The hard way looks like it would be more correct, but could also break
compatibility with other software.  I don't know who uses libmp other
than the rest of userland, however, so I would like to ask if there
are any other users at all, and if so, if you object to changing
libmp.  I have a need for this and can put a patch together if this
change seems okay.

Re: [9front] libmp and error handling

[Alex Musolino]

> There's both an easy way and a hard way to fix this.  The easy way
> would be to document the sysfatal call in the manpage and mention
> that sysfatal should be overridden in order to change the error
> handling.  The hard way would be to remove the sysfatal calls in
> libmp, then change the rest of the system so that it catches the
> errors.

Or split the difference and introduce something like Blethal(2).  Then
change the existing programs (less than 40 by my count) to supply
sysfatal as the handler.

Re: [9front] libmp and error handling

[covertusername967]

> Or split the difference and introduce something like Blethal(2).  Then
> change the existing programs (less than 40 by my count) to supply
> sysfatal as the handler.

This is a better idea.  Even better, just leave the other programs as
is and make sysfatal a default.  Thanks!

Re: [9front] libmp and error handling

[ori]

> This is a better idea.  Even better, just leave the other programs as
> is and make sysfatal a default.  Thanks!

This is still likely to require quite a bit of change in libmp; if
it's currently calling sysfatal, then it's very likely full of
things like:

	x = a();
	y = b()
	return mpadd(x, y);

which means that if b() errors, then x is leaked.

Re: [9front] libmp and error handling

[covertusername967]

> This is still likely to require quite a bit of change in libmp; if
> it's currently calling sysfatal, then it's very likely full of
> things like:
> 
> 	x = a();
> 	y = b()
> 	return mpadd(x, y);
> 
> which means that if b() errors, then x is leaked.

So, I tried changing libmp itself.  It rapidly became a hairy mess of
return checking, and making the library do more allocation just to
preserve the original function arguments.  Instead, here's a patch to
fix the manpage:

diff -r 5a6b0e6ac3b2 sys/man/2/mp
--- a/sys/man/2/mp	Sun Oct 25 00:49:29 2020 +0200
+++ b/sys/man/2/mp	Sat Oct 24 18:51:53 2020 -0500
@@ -287,8 +287,8 @@
 .I mpnew
 and
 .IR mpcopy ,
-will return
-.B nil
+will call sysfatal (see
+.IR perror (2))
 if the allocation fails.
 .PP
 Input and result parameters may point to the same

Re: [9front] libmp and error handling

[ori]

>> This is still likely to require quite a bit of change in libmp; if
>> it's currently calling sysfatal, then it's very likely full of
>> things like:
>> 
>> 	x = a();
>> 	y = b()
>> 	return mpadd(x, y);
>> 
>> which means that if b() errors, then x is leaked.
> 
> So, I tried changing libmp itself.  It rapidly became a hairy mess of
> return checking, and making the library do more allocation just to
> preserve the original function arguments.  Instead, here's a patch to
> fix the manpage:

Thanks, committed.