[erlang-bugs] enif_make_badarg crashes is_binary

[Steve Vinoski]

On Tue, Feb 22, 2011 at 5:27 AM, Sverker Eriksson
<sverker@REDACTED> wrote:
> Mikael Pettersson wrote:
>>
>> Steve Vinoski writes:
>>  > I was recently writing a nif and had one C function returning an
>>  > ERL_NIF_TERM that was then being passed to a second C function. The
>>  > second one checked the passed term using
>>  > enif_inspect_iolist_as_binary(), and it crashed. I debugged it and
>>  > it's apparently caused by the fact that the ERL_NIF_TERM being passed
>>  > was returned from enif_make_badarg(), so it was a 0. It caused this
>>  > error on my Macbook Pro:
>>  >  > Program received signal EXC_BAD_ACCESS, Could not access memory.
>>  > Reason: KERN_INVALID_ADDRESS at address: 0xfffffffe
>>  >  > The reason for that invalid address is the is_binary(x) macro at the
>>  > beginning of enif_inspect_iolist_as_binary(). It expands to
>>  >  >     if (((!(((((term)))) & (0x3 -0x2))) && (((*((Eterm*)
>> (((((term)))
>>  > - 0x2)))) & (0x3F -(0x3 << 2))) == (0x0|(0x8 << 2))))) {
>>  >  > The first test, which I believe comes from _unchecked_is_boxed(x),
>>  > passes because it's testing !0 which of course is 1, then the second
>>  > test which I believe is _unchecked_boxed_val(x) fails with the invalid
>>  > address error because the code more or less dereferences a null
>>  > pointer.
>>  >  > I think the fix is to make macros like is_binary(x) first check the
>>  > term using is_value(x).
>>
>> No.  Non-values are emphatically NOT permitted inside Erlang process
>> heaps, live BEAM VM registers, live terms, or as parameters to internal
>> C functions (except for very few special cases).  The non-values are
>> very private to the VM, and are (as I recall, it's been a while since
>> I wrote that code), limited to (a) indicating exception returns from
>> C BIFs (handled immediately by beam_emu), (b) temporary magic markers
>> during GC, and (c) signalling error returns from some internal C
>> functions.  In no case is a non-value allowed to leak into
>> application-visible Erlang state.
>>
>> The fix, therefore, is to catch whatever produced that non-value
>> (error from a C function?) and turn that into an error return or
>> Erlang exception rather than trying to pass it on as a valid term.
>>
>> /Mikael
>>
>>
>
> Yes, Mikael is right.
> The documentation could be more clear about the only valid use of
> enif_make_badarg(); return its value from the nif. You are not even allowed
> to change your mind, once enif_make_badarg() has been called, its return
> value *must* be returned from the nif.
> enif_make_badarg() was a bit of a quick fix just to get the most common use
> case to work. In future we might want functions to throw customized
> exceptions. A small step in that direction could be an enif_is_value() (or
> maybe enif_is_exception) in order to  distinguish values from exceptions.

Thanks, Sverker and Mikael, this makes sense. I didn't bother trying
to create a patch for this because I didn't feel I understood the
original intent.

I think enif_is_exception() could be very handy. It would be perfect
for the code I described in my initial message, since it would allow
me to check the return value from the secondary function to see if it
was an exception and if so return it immediately from the primary
function. Not having enif_is_exception() wasn't too hard to work
around for my case since badarg was all I needed and so I could just
return a success/fail from the secondary function instead and create
and return the badarg from the primary function, but if more
exceptions are added in the future, enif_is_exception() will almost
certainly be needed.

thanks,
--steve