Fix allocation and minor memleaks in json path#5
Fix allocation and minor memleaks in json path#5lynxis wants to merge 2 commits intoopenwrt:masterfrom
Conversation
jp_get_token() is using a stack allocated jp_opcode op and is not using jp_alloc_op() or jp_free(). jp_get_token() is calling match_token() which might assign op->str and allocate it via strdup(). Fixes: TOB-OWRT-3 Reported-by: Trail of Bits Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
jp_alloc_op() is not only allocating the memory of the jp_opcode itself, but might allocate also op->str as a whole block by a single calloc() call. However match_token() might also assign op->str using strdup(). This might result in further memory problems, as the different memory allocation strategy happens on the same member. Remove this optimisation of allocating the jp_opcode and op->str from the same block, to ensure it is used everywhere in the way same. Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
|
Why is the simplification needed? The idea of the calloc_a() call was to store the string copy and the bookkeeping data in the same heap block, to avoid extra allocations which might lead to additional memory fragmentation. Probably not world moving either way but why drop the optimization if it already is present and working? |
|
@jow- yes the current code is working and I see the point in the optimisation of the a single calloc block. lexer.c is assigning So depending where the object comes, the op->str requires to be freed or not. It would be also fine for me to drop the commit removing this optimization. |
| newop = jp_alloc_op(s, op.type, op.num, op.str, NULL); | ||
| /* match_token might alloced str using strdup() */ | ||
| if (op.str) | ||
| free(op.str); |
There was a problem hiding this comment.
Couldn't you just
| newop = jp_alloc_op(s, op.type, op.num, op.str, NULL); | |
| /* match_token might alloced str using strdup() */ | |
| if (op.str) | |
| free(op.str); | |
| newop = jp_alloc_op(s, op.type, op.num, NULL, NULL); | |
| newop->str = op.str; |
and avoid the strdup() in jp_alloc_op()?
3 participants
Fix allocation and minor memleaks in json path