jansson
jansson copied to clipboard
akheron
arguably-inappropriate assert during low memory conditions
I'm using jansson in an embedded application with, of course, constrained memory. It is not unexpected that the allocator will occasionally return errors before it has a chance to flush, retry, etc. As such, i'm relying upon a) jansson's super clean error handling behavior (which is quite well done), and b) the fact that the processor will never take a fault. It's embedded, after all.
I may be wrong - apologies if i don't get this exactly right - but I believe I just ran into a case in which there is an assert taken on a "normal" out of memory error path. I believe it is as simple as:
- be doing a lex_scan_number in the normal course of operations
- this does repeated lex_get_save -> lex_save -> strbuffer_append_byte
- strbuffer_append_byte fails because of a failed malloc to expand the buffer
- because lex_save's call to strbuffer_append_byte does not do any error checking, everything proceeds quietly
- when hitting a delimiter, lex_scan_number ultimately calls lex_unget_unsave
- lex_unget_unsave sees that 'c' is the delimiter (in my case a comma), while 'd' from strbuffer_pop is the last thing in the lex->saved_text (in my case a '0'), presumably because the append had never happened.
- we take the assert(c == d) for obvious reasons, and hilarity ensues on the embedded device.
Obviously had errors been bubbled out of lex_save and all its callers, the failure would have been caught sooner. But, regardless, my recommendation would be to remove the assert if indeed you believe that the package should be just taking "normal" error paths in low-memory conditions.
Well, there may be more problems than that. I just debugged a massive memory overwrite that appears to be the following:
- come into lex_scan_string from lex_scan after saving a "
- enter a loop that iiteratively calls lex_get_save, looking for the trailing quote
- somewhere during the middle of the string, there is a malloc failure within the aforementioned strbuffer_append_byte within lex_get_save. But because of no error path taken, the 'stream_get' works just fine but the lex_save does not save it into the saved_text.
- eventually, the while loop finds the terminating " in the stream and drops out
- the jsonp_malloc attempts to allocate a copy of the saved_text (which i can see in the debugger does not have a trailing quote; it is a valid string from my json that prematurely ends). This alloc succeeds; it is very small (16 bytes).
- after the malloc, we enter a second loop that copies the entire string with "while(*p != ':')" ... "*t++ = *p++", without any bounds checks on the pointers. The loop was clearly written under the assumption that it WOULD find the trailing " (because it found it in the stream), however since there is no trailing quote in the saved_text it proceeds to overwrite memory. In my case it overwrote quite a lot before causing a hard fault.
For the next person encountering this, I've made the following changes FYI and we'll see how well it perform. So far so good.
- in pack_unpack.c, added strbuffer_init as well as strbuffer_append_bytes error handling
- in load.c, a) changed lex_save, lex_save_cached, and lex_scan_string to return ints (0/-1) vs being void b) lex_save now returns result of strbuffer_append_byte c) in lex_get_save, if there is an error on lex_save, now set c = STREAM_STATE_ERROR so that the get fails if the save failed d) lex_save_cached returns -1 if lex_save fails, else 0 e) in lex_scan, added a TOKEN_INVALID error path now that lex_scan_string returns an error f) changed each and every occurrence of lex_save, lex_get_saved, and lex_save_cached to check their return arguments and to do the obvious thing for error handling.
