Invalid run-time allocation of array in rtos_tasks.c

[JohnathanBurchill]

In Core/Src/rtos_tasks/rtos_tasks.c near line 124 in the main branch:

                    uint8_t args_str_no_parens[end_of_args_idx - start_of_args_idx + 2];

Run-time allocation of an array is normally done on the heap via malloc(<array_size>) et al. Or a maximum array size is allocated with a fixed size, like

#define MAX_ARG_LIST_SIZE 512

...

                   uint8_t args_str_no_parens[MAX_ARG_LIST_SIZE];

and the actual size used has to be tracked.

I saw somewhere on discord that malloc is not wanted in firmware code. If so, the second form above should be used.

(wondering why the existing code did not trigger a compiler #warning ...)

[DeflateAwning]

Thanks for bringing this up! It's funny, because there's actually a bug in that section you pointed out regarding the null terminator being written at the wrong byte index at the end of that array (which perhaps was what brought you there).

My understanding is that variable-length arrays are allocated on the stack still (and thus not in the heap). My understanding is that malloc (or a function which calls malloc) is the only way to allocate memory on the heap.

Here's a Stackoverflow post asking about this feature. Looks like most of the answers back up this understanding: https://stackoverflow.com/questions/56533887/creating-a-variable-sized-array-without-malloc

We do indeed want to avoid heap allocation, and I'm planning on looking into turning it off in the FreeRTOS init. The idea with this is that we want to avoid accidental bugs which frequently occur as a result of improper heap use (use-after-free, memory leaks, etc.). We also want to ensure memory usage is reasonably predictable.

Arrays declared outside of functions are allocated in the "data segment"; they're nice because it can't result in a stack overflow, but it's a "global" variable, and thus requires special care and attention to avoid unpredictable access.

In summary, I think this is optimal in its current state, but am absolutely open to discussion if I'm missing something or if anyone has other insight!

[JohnathanBurchill]

Ah - VLA's, right - I'm from the C90's!

[JohnathanBurchill]

I'm comfortable with malloc and free, which can be made robust with a valgrind-type of analysis, but will stick with globals and stacks for the firmware, unless there turns out to be a case where it is unavoidable. You will have to restrict usage of other functions, like strdup for example.

[JohnathanBurchill]

Is there a rule for deciding when an array is too large for the stack?

[JohnathanBurchill]

Thanks for bringing this up! It's funny, because there's actually a bug in that section you pointed out regarding the null terminator being written at the wrong byte index at the end of that array (which perhaps was what brought you there).

Indeed, I caught that earlier and was prepping a bug report. Glad to see that has been spotted and a fix is in the works. I was trying to pass fs_read_file(testfile) but would get an error reading "testfile^A" as a response.

[JohnathanBurchill]

Another downside to malloc and free is the potential for a performance drop. Not sure if it would matter, as that can often be optimized, but something to consider.

[DeflateAwning]

The advice I've read a lot of, and the advice I internalized about a year ago when reading a doc on "how NASA has strong rules about C to increase its safety for space applications", avoiding malloc was one of the pieces of advice.

The NASA doc is summarized here.

Relevant (to this discussion) rules from there [with my comments in square brackets]:

• Do not use dynamic memory allocation after initialization. [which I take to mean heap allocation, although I suppose VLAs do fall in this category; perhaps we should be allocating the maximum allowable size for those arrays]
• Declare all data objects at the smallest possible level of scope. [that is, even though global/static arrays are tempting, they're very very accessible]

We have 640 KB of RAM. The UART telecommand executor thread currently has a max stack size of 8196 bytes (per the thread config in main.c. Thinking about mission requirements, I don't forsee needing more than 2-3 1024-byte arrays in a stack ever. I'd hazard a guess that the recommended practical maximum on stack and global (well, static; that is, outside of a function) array allocation on this sort of system probably falls in the 10-16 KiB range, although I can't think of a reason you couldn't allocate far more even, so we should be okay.

It's good I took another look at that NASA doc, as there're a few things we could do better.

[JohnathanBurchill]

It's good I took another look at that NASA doc, as there're a few things we could do better.

Like "... and do not use function pointers."

I'm fine with function pointers, but then I don't write software for NASA :)

I do think it is good to be aware of these recommendations and to take them as guidelines. But are we going to be doing static analysis of the firmware?

[DeflateAwning]

I setup Doxygen, which generates call graphs. I think that'll probably be the extent of static analysis we have planned.

I'm happy with the "registration of the functions in a table" approach that we're using functions pointers for currently (that is, only telecommands inventory and unit tests inventory), but I do think we should avoid using them anywhere else, as these function inventories are really the only ones that make sense.

[DeflateAwning]

Reading into it more, the static analysis you propose here is more about determining the max memory usage of a thread, presumably. I think that's a great thing to look into! I suppose that's the exact case the function pointers break down; will cross that bridge in a bit. Would be an easy-enough refactor (with computer-generated code) if it becomes a limitation.

[JohnathanBurchill]

You address the actual issue by pointing out the use of VLAs. I'm closing this.