Flux-core: jansson: safe error handling

[dongahn]

As I'm adding Jansson error handling code, I see numerous Jansson error handing patterns in flux-core like this.

I keep asking myself how safe the following json_decref statement would be. While this will prevent a memory leak, depending on how json_object_set_new fails, the subsequence json_decref statement could lead to an invalid read.

Did some of you discussed this before?

[chu11]

@dongahn Is your concern that json_object_set_new() could add val to the object but still error? Thus there could be a double free?

I think there's always been the assumption that json_object_set_new() will not set val on any type of error. But perhaps that's a bad assumption?

[dongahn]

@dongahn Is your concern that json_object_set_new() could add val to the object but still error? Thus there could be a double free?

Yes.

I think there's always been the assumption that json_object_set_new() will not set val on any type of error. But perhaps that's a bad assumption?

I didn't look at json_object_set_new code, and this wasn't clear at all from the reference page. But if this assumption is violated, there will be access violation.

BTW, json_object_set_new is just one function. There are other json functions including json_pack, json_append_new etc and I wasn't sure if I make this assumption for every such call...

[grondo]

The _new functions steal a reference to their argument. If the function fails then the reference is not stolen, so you need a decref. If the Jansson function fails, but still did steal the reference then that is a bug in the function. I'm not sure if defensive programming against all possible bugs in other software is too practical, though...

[grondo]

But @dongahn, you are right that the docs are definitely not clear.

[dongahn]

Hmmm... just out of curiosity, I looked at one code site (https://github.com/akheron/jansson/blob/master/src/value.c#L125), and it looks to me like json_object_set_new() calls json_decref on the object for all of the failure paths by itself, though? I didn't look at other functions. (Need to work on this 2 pager thing I need to submit by tomorrow.)

[garlick]

Uh oh, I think you are right @dongahn!

On Thu, Mar 5, 2020, 10:12 PM Dong H. Ahn [e-mail] wrote:

Hmmm... just out of curiosity, I looked at one code site (
https://github.com/akheron/jansson/blob/master/src/value.c#L125), and it
looks to me like json_object_set_new() calls json_decref on the object
for all of the failure paths by itself, though? I didn't look at other
functions. (Need to work on this 2 pager thing I need to submit by
tomorrow.)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/flux-framework/flux-core/issues/2803?email_source=notifications&email_token=AABJPW64WOVMEKZP7RM2QV3RGCH4BA5CNFSM4LCY65HKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOAGKVY#issuecomment-595617111,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABJPW4LSGFYYO3IJJ5T43TRGCH4BANCNFSM4LCY65HA
.

[grondo]

Wow that's terrible (to have to look at implementation to figure this out)! Sorry, @dongahn!

[chu11]

Looking through jansson's code, json_array_set_new() and json_object_iter_set_new() also apply here. Which we use all three of these in flux-core.

[grondo]

Yeah, it hadn't even occurred to me that this would be the case, and therefore I have to apologize to @dongahn for my comment above. :woman_facepalming:.

I guess one can imagine the Jansson author(s) did this to ease the cleanup burden on callers, but for some reason it feels a little wrong. Maybe just too much time thinking the other way around. The patch to fix this should remove a lot of untested lines of code so that's good I guess!

[dongahn]

@chu11: how about json_pack? We use this even more frequently?

[dongahn]

Yeah @grondo: Less code in error paths the better increasing our coverage :)

[garlick]

I've internalized the idea that functions should have no side effects on failure, so this was surprising to me!

[grondo]

I've internalized the idea that functions should have no side effects on failure, so this was surprising to me!

Yeah, that is what is bothering me too.

[dongahn]

I agree. And if they did, at least this should be documented in particular those public ones.

[chu11]

@chu11: how about json_pack? We use this even more frequently?

Good call, I totally forgot about that.

The code isn't the most obvious on json_pack(), but if I've figured it out correctly, it can SOMETIMES free values and sometimes not. SCARY! Has to do with the fact it iterates through the entire format (i.e. { s:i s:o s:O s:f }) and depends on the error and where it occurs.

[dongahn]

Seems we shouldn't free it to be safe. A leak has less side effect than double free. And this would happen only on a failed path so this shouldn't lead to massive leaks.

[grondo]

And this would happen only on a failed path so this shouldn't lead to massive leaks.

Be careful though, a reproducible leak due to bad formatted JSON payload to a service could be a DoS vector. I agree better than double free, but this is unsatisfying.

[grondo]

This might be worthy of a bug opened against jansson??

[chu11]

I'm trying to write up a small reproducer ... see if what I'm observing is true.

[dongahn]

Yeah, a bug report makes sense to me. At the very least they should "consistent" error handing semantics.

[dongahn]

There is also json_array_append_new, which frees it by itself.

[chu11]

So far I've been unable to reproduce (atleast with version installed on TOSS3). If I'm reading code correctly, I need to generate an error earlier in the format string, but have it continue to process later on in the format string. Something like:

json_pack("{ s:o s:o }", "a", NULL, "b", o);

But valgrind isn't finding a double free. So maybe I'm reading the code wrong.

At the minimum, json_pack() should be less concerning since its not a consistent mem-leak/double free issue (like the set_new functions), it requires the format to be done in a specific way.

But perhaps bringing up the question for jansson is a good idea.

[dongahn]

At the minimum, json_pack() should be less concerning since its not a consistent mem-leak/double free issue (like the set_new functions), it requires the format to be done in a specific way.

So the recommendation is to free in our code or not? My guess is not free? I am writing this code right now so a feedback would be helpful.

[chu11]

So the recommendation is to free in our code or not? My guess is not free? I am writing this code right now so a feedback would be helpful.

IMO I would free, since with my trivial example above, there is no free happening in json_pack().

[dongahn]

Got it.

[chu11]

Posted a message to "https://groups.google.com/forum/#!forum/jansson-users" if anyone is curious, since it appears that's more active than github issues

[chu11]

Just as a summary, the functions to audit for use are:

int json_object_set_new(json_t *object, const char *key, json_t *value);
int json_object_set_new_nocheck(json_t *object, const char *key, json_t *value);
int json_object_iter_set_new(json_t *object, void *iter, json_t *value);
int json_array_set_new(json_t *array, size_t index, json_t *value);
int json_array_append_new(json_t *array, json_t *value);
int json_array_insert_new(json_t *array, size_t index, json_t *value);

It's used in quite a lot of places ... :-(

[chu11]

FWIW, no one responded to my post in the mailing list. I suspect b/c no one knows the true answer on how to deal with it for json_pack() :-(

[garlick]

I guess failure of these functions is sufficiently unlikely that this is a low priority, but as a workaround, we could trivially add internal, "safe" versions of at least the non-variadic functions mentioned above, e.g.

int safe_json_object_set_new (json_t *object, const char *key, json_t *value)
{
    if (json_object_set (object, key, value) < 0)
        return -1;
    json_decref (value);
    return 0;
}

Then updating the code base becomes a search and replace.