fix: decref deletes json_t val even if refcount == 0

[denpun]

json_decref will attempt to decrease refcount even if refcount == 0 and subsequently try to delete an already deleted json_t value. This can cause a double free.

This fix simply returns from the json_decref function without doing anything, if refcount == 0, i.e. already decref-ed and deleted.

[denpun]

I checked the failed tests. It seems my understanding of the decref logic is a bit off.

json_decref is not supposed to be called twice. < This is understandable.

If it is indeed called twice, can we have way to simply ignore as opposed to doing the delete anyways and causing a double free?

I will review the code properly to fully understand current decref logic.

I guess by having it delete, it forces programmer to ensure their use of json_decref is correct and understand library properly. This is also understandable.

I looked at this because of the json_object_set_new(obj, key, value) function where we had originally assumed that if it fails, we are to decref the value passed to the function ourselves. It seems the json_object_set_new function always call json_decref. Maybe we can just document the fact that json_object_set_new() will always decref no matter whether it is successful or not.

[coreyfarrell]

It is documented at https://jansson.readthedocs.io/en/2.12/apiref.html#c.json_decref:

However, some functions steal the reference, i.e. they have the same result as if the user called json_decref() on the argument right after calling the function. These functions are suffixed with _new or have _new_ somewhere in their name.

You are seeing the 'double-free' error likely because libc crashes on it, but just reading refcount === 0 is use-after-free which is also a serious error though it doesn't necessarily result in a crash (but could in some situations). use-after-free is something which valgrind should report, and should be fixed as it is a serious memory management bug.

[denpun]

but just reading refcount === 0 is use-after-free

Oh yes. Silly me. :)

Got distracted. Forgot refcount is part of the json_t val.

It is documented at https://jansson.readthedocs.io/en/2.12/apiref.html#c.json_decref:

Ah. I see that now. I guess I kept looking in the set_new fun doc.

use-after-free is something which valgrind should report, and should be fixed as it is a serious memory management bug.

Agreed.

Thanks for clearing it up. Things are a little clearer now after I went back and really looked through the code. For some reason I thought the refcount is separate. Note to self, don't try to fix bugs on a late Friday evening! :) Have a good weekend.