Thanks for the various fixes.

In general, I try to keep the code short and I prefer to have memory leaks if the program is going to end than making a very long and verbose code. There are some of your fixes that I might include in the future, though. I also prefer to make these changes myself. If you are willing to suggest changes, I prefer issues to PR for this reason. Closing.

I agree that memory leaks in a program that runs once may be OK. Not checking for malloc returning NULL is not a good idea, maybe a wrapper that aborts will help you to keep to code short and still make it safe.

What was important in this PR is commit f59f224...

I will have a look at the malloc thing, thanks for your suggestion. It is such a controversial topic (I found this interesting discussion about it https://stackoverflow.com/questions/763159/should-i-bother-detecting-oom-out-of-memory-errors-in-my-c-code).

Regarding the commit f59f224, I don't see any bug in there. Indeed, the program is usually allocating more memory than it will need, and it's also true that if tmp1 and/or tmp2 are bigger than 6 characters, then the program may perform out of bound access. But I would say that this would be a bug in get_value_as_smallest_unit, not in get_str_cache_two.

However, in the end, I agree that this part is not well designed. I will change that and review similar parts of the code to find similar issues. Thank you very much for your time reviewing this!

About commit f59f224:

uint32_t max_size = 4 2   2   4 2   7   1;
char* string = malloc(sizeof(char) * max_size);

then

uint32_t size = tmp1_len   2   tmp2_len   7   1;
sanity_ret = snprintf(string, size, "%s (%s Total)", tmp1, tmp2);

With this size becomes bigger than max_size and snprintf accesses memory beyond the end of the buffer.
This only happens on certain CPU models where the length of tmp1 and tmp2 becomes more than 12 - I have a machine with such CPU but do not remember exactly which one was it.

This is not reported unless the program is built with _FORTIFY_SOURCE=2; only then the program prints:

*** buffer overflow detected ***: terminated
Aborted

BTW. _FORTIFY_SOURCE=2 is part of the default build options on most modern distributions - I would not consider that one dangerous in any way...

I understand your point, but the situation in which tmp1 and tmp2 becomes more than 12 is only possible if one (or both) has a length bigger than 6 characters. Honestly I am unable to find a value that can cause that get_value_as_smallest_unit returns more than 6 characters. In the case of the biggest cache size possible, 2^31 (since I'm using a int), then the function returns "2048MB", which is 6 characters long.

Can you post the CPU model which caused the issue? Thanks.

Sorry, I couldn't find the exact server where it aborted but the example above is showing that 13.75MB is 7 chars... Maybe it was AMD?

Anyways - I see no point guessing the size - regardless if the calculation is right or wrong, the value used in snprintf should be exactly the same as the one used in malloc. Doing something else and assuming things makes the use of snprintf pointless - it would be better to add a comment that the size is 'properly' guessed and use sprintf :)

As I look into get_value_as_smallest_unit it uses uint32_t and the biggest result comes from 2^32-1 which would be 4096MB; but there is no reason not to get 1234.678MB (smaller value but longer string) that would make the returned string length 10 (which is properly sized there) but will obviously break the caller.

Yeah, I agree, that is why I said that I am going to redesign this part.

These days I have been doing stuff in different parts of the code and I have detected many cases like this (bad designed code). Some parts of the code are old, or they were just not very well done from the beginning. I will go through the code to fix these small issues. And I will also consider adding _FORTIFY_SOURCE=2.

Thanks for your tips!