Result of zpl_alloc() is often not checked

[rheatley-pervasid]

Sorry to bombard you with all the issues! It's honestly because I am enjoying using the library so much for all sorts of projects :)

The following code seg faults

char buffer[256];
zpl_arena arena;
zpl_arena_init_from_memory(&arena, buffer, sizeof(buffer));
zpl_allocator allocator = zpl_arena_allocator(&arena);
zpl_adt_node root;
zpl_adt_make_branch(&root, allocator, "#", 0);

This is because zpl_alloc fails in zpl_array_init_reserve (from zpl_array_init, from zpl_adt_make_branch)

#define zpl_array_init_reserve(x, allocator_, cap)                                                                     \
     do {                                                                                                               \
         void **zpl__array_ = cast(void **) & (x);                                                                      \
         zpl_array_header *zpl__ah =                                                                                    \
         cast(zpl_array_header *) zpl_alloc(allocator_, zpl_size_of(zpl_array_header) + zpl_size_of(*(x)) * (cap)); \
         zpl__ah->allocator = allocator_;                                                                               \
         zpl__ah->count = 0;                                                                                            \
         zpl__ah->data = (char *)x;                                                                                     \
         zpl__ah->capacity = cap;                                                                                       \
         *zpl__array_ = cast(void *)(zpl__ah + 1);                                                                      \
     } while (0)

As you can see zpl__ah is used without a null check.

There seem to be quite a few instances where the failure is not checked. I'm hoping you agree it should return an error rather than segmentation fault!

I'm happy to try and fix the related issues and submit a PR if you like. Let me know your thoughts.

(In my use case I am on an embedded device so prefer static allocation, hence zpl_alloc can fail without the heap being exhausted)

[zpl-zak]

If you can provide a PR for this one that would be great! Thanks again for all the reports, I appreciate the help!

[rheatley-pervasid]

@zpl-zak - a significant trickiness to the change is the problem occurs a lot in _init calls which all have the same design pattern of being a large do/while in a #define - there isn't really a good way to return failure.

I'm minded to suggest I add _alloc functions which return the type being created, similar to zpl_adt_alloc. I guess that would then mean the _init calls could be marked deprecated but remain (for now/ever) for API stability.

I didn't anticipate causing that much of a change! I am very open to any suggestions from you regarding which direction I set of in.

[zpl-zak]

Sadly that would be difficult to change as many other modules rely on an allocation that would be affected.

However, since we know this issue is related to containers (zpl_array, zpl_buffer), I suggest we expand the header structure of these containers and add an error code field that would get set in case we fail any allocation. Users can then refer to this field to check for OOM errors and handle them appropriately, which is a viable alternative to returning by error code.

The user, in this case, would be the parser/adt code that would then validate this error code and return the appropriate error to the library user.

EDIT: I can address this tomorrow, and I will use your demo code as a test case to validate the bad path in our UTs. Once I get the branch ready, we can discuss if this is a viable path forward.

[rheatley-pervasid]

I'm not sure the knock-on effect is too bad. I guess if zpl ends up checking NULL everywhere, each module is getting updated one way or another. Neither approach requires that to happen today, I was just trying to leave a pattern that could be used each time later on.

Here is the proposed change https://github.com/zpl-c/zpl/compare/master...rheatley-pervasid:zpl:add-alloc-call

I couldn't see a good reason why the array held onto the original pointer value, so removed it for simplicity.

(There is an unnecessary fix to zpl_strdup in there as well)

[zpl-zak]

I see, yes this approach would work well in that case. I agree performing the alloc in the macro is flawed due to no error handling being present, luckily with your approach, it shouldn't break backward compatibility indeed. Let's continue going down this path.

[rheatley-pervasid]

I think arrays are complete https://github.com/zpl-c/zpl/compare/master...rheatley-pervasid:zpl:add-alloc-call

Because I cannot rely on sizeof() for the array type, elem_count has got cached. I couldn't understand how capacity could ever be less than count which was a special case in array.c If you agree, the recursive call is unnecessary and I inlined the function.

If you are still happy with this direction I will progress it over the next week.

[zpl-zak]

Looks good to me; thanks again for the contribution! 👍🏻

[rheatley-pervasid]

Been a lot busier than anticipated, so a bit delayed finishing off the arrays work.

I came to the conclusion that API pattern wouldn't work well when the array got reallocated underneath us.

So I've had a second go, https://github.com/zpl-c/zpl/compare/master...rheatley-pervasid:zpl:alloc-take-2 Essentially all existing "functions" are now really functions and return true if success, false if allocation failed. zpl_array_append and zpl_array_append_at are the least nice in terms of implementation. So let me know if too magical!

p.s. there is at least a bug in zpl_array_appendv_at not involving ind in the zpl_memmove number of bytes to move

[zpl-zak]

I think this approach will work out fine in the end. I like how things look so far, thank you again!

[rheatley-pervasid]

Hi @zpl-zak - been quite busy so this got a bit neglected. I think I am happy with where I left it last - https://github.com/zpl-c/zpl/compare/master...rheatley-pervasid:zpl:alloc-take-2

Would you like anything tidying up or I'll create a PR? json.c looks to have had a lot of whitespace in, which my editor stripped. I can restore it if you want a more minimal diff.

[zpl-zak]

Hi @rheatley-pervasid, I will get back to it this upcoming weekend. Sorry for the delay!

[zpl-zak]

@rheatley-pervasid Sorry again for disappearing! I think it looks fine, could we also expand it to zpl_buffer and other macro-based collections? If not that's fine too, we can go ahead with PR and I can follow your changes and apply them to other collections. Thanks for the contrib!

[rheatley-pervasid]

@zpl-zak no worries, it seemed optimistic to look at it much over Christmas :) I'm very happy to keep working through other non-ideal allocs. I'm tempted to say I'll make a PR for this in isolation

• if I have broken anything, spotting it in a smaller commit should be easier
• selfishly the JSON allocations are more time critical for our project (we still use the buffers elsewhere in the code, so fixing still on my radar!)

[zpl-zak]

Hi, I'm sorry I am no longer current on this issue. Are we good to close it now, or are more actions required?

[rheatley-pervasid]

Hi @zpl-zak - it is up to you really. There are still several locations where zpl_alloc() is not checked (string.h, buffer.h) - but I think I fixed all the cases that could occur from JSON serialisation/parsing. So either, it can be a placeholder for the other issues, they can get their own issue, or it is all forgotten about for a bit!

(I was fixing some more stuff here, https://github.com/rheatley-pervasid/zpl/commits/alloc-improvements - but I've been very short of time recently, so I doubt I will make any more progress in the short term)

[zpl-zak]

Thanks for the response! I will look into the mentioned branch, apply the fixes to the main repo, and continue down this path to cover more locations. Thank you for the help so far. No worries, it does not hurry. I'll keep this issue open for tracking purposes.