Practise problem on C function about fixing errors

Question

I am practising for a test where there are debugging questions. For the code below it says there are 2 errors inside the function, but it doesn't seem like there is an error plus i tried running it and it does work fine (the code given is intended to look weird but I'm pretty sure it functions correctly). Is there maybe something that will make an undefined behaviour or error in the question?

Question:

The following function takes an integer array and the length of that array. It should calculate the sum of that array and return the computed value. Identify and explain the two errors in this function and explain how to fix them.

int sum(int nums[], int nums_size){
    int *total;
    if((total = (int *) malloc(sizeof(int))) == NULL){
        exit(1);
    }
    for(int i = 0 ; i < nums_size; i++){
        *total += nums[i];
    }
    return *total;
}

Don't confuse "errors" as in "compiler errors" with "errors" as in "mistakes in the code". — tadman, Dec 27 '20 at 02:18
exist(1) is a poor error handling strategy. @costaparas why don't you write your suggestions as an answer. You nailed it. — Allan Wind, Dec 27 '20 at 02:19
This "works fine" by a very narrow definition of "fine" which apparently includes "leaks memory" in its definition. This allocation appears to be completely pointless, and `int total` should work just as well. — tadman, Dec 27 '20 at 02:19
Also, if this is c99 then a better signature would be int sum(int nums_size, int nums[nums_size]). Is nums_size < 0 valid? — Allan Wind, Dec 27 '20 at 02:28
I would also push back on the function existing in the first place, i.e. int total = 0; for(int i = 0; i < nums_size; total += num[i++]). — Allan Wind, Dec 27 '20 at 02:34
After reading all the comments, I would say that the two errors are `not initializing the total to 0` and `and working with an a pointer to int rather than just an int` — anotherOne, Dec 27 '20 at 02:51
@AllanWind *`exist(1)` is a poor error handling strategy.* Why? Assuming you mean `exit()` is a poor error handling strategy, I'll point out that any code that can't properly recover from a call to `exit()` also can't properly recover from an unexpected power outage or kernel crash or sysadmin killing the wrong process or a runaway OOM killer or a SIGSEGV or any other issue that results in immediate death of the process. In short, if it can't handle `exit()`, it's crap that has no business running in production. — Andrew Henle, Dec 27 '20 at 02:59
@AndrewHenle ... because the only safe way to call the function is in a separate process and it assumes that all your cleanup code has been registered with atexit(). This usually precludes the code from use in a library. I understand what you saying, of course, but not sure the examples you give are particular relevant. malloc, presumably failed due to lack of memory, and caller may want to have the option to free some. — Allan Wind, Dec 27 '20 at 03:21

score 3 · Accepted Answer · answered Dec 27 '20 at 02:51

The "two" errors the question probably intended you to identify are:

The code does not initialize *total, only total. Hence, uninitialized memory is accessed and the behavior is undefined, even if it appears to "work" in some cases.
The code causes a memory leak - when the function returns, it returns an int and the pointer to that memory is lost, so cannot be freed.

How to resolve these two bugs:

Set *total = 0 just before the for loop.
Avoid using malloc (which is completely redundant) and instead use a regular int declared on the stack.

Here is a corrected version:

int sum(int nums[], int nums_size) {
    int total = 0;
    for (int i = 0 ; i < nums_size; i++) {
        total += nums[i];
    }
    return total;
}

Here is one possible solution you can use if keeping the call to malloc is desired. As you can see, its a bit silly and clearly shows it is redundant here. Also, I've removed the cast of malloc's return value - don't do it, see more details in this post.

int sum(int nums[], int nums_size) {
    int *total;
    if ((total = malloc(sizeof(int))) == NULL) {
        exit(1);
    }
    *total = 0;
    for (int i = 0 ; i < nums_size; i++) {
        *total += nums[i];
    }
    int ret = *total;
    free(total);
    return ret;
}

Also, another possible issue is signed integer overflow.

For example, the total could easily overflow if we consider the array:

{999999999, 999999999, 999999999}

I doubt the expectation here is to resolve this issue or handle it in some way. There are some other posts on Stack Overflow that make suggestions on this topic if you're interested, e.g. this one.

Tip: valgrind is a powerful tool -- and would've easily detected both the intended bugs in the code in this case (uninitialized memory use and the memory leak).

Accessing uninitialized memory does not have *undefined behavior* as defined by the C standard. The value of an uninitialized object is *indeterminate*. — Eric Postpischil, Dec 27 '20 at 10:48

Allan Wind · Answer 2 · 2020-12-27T03:42:21.907

Let the calling code malloc & free the variable total, and do it at the same level (stack frame):

int *total = malloc(sizeof(int));
*total = 0;
sum(nums, nums_size, total);
free(total);

This is general good strategy to avoid memory leaks, also, it gives the caller the option to store the result on the heap (malloc) or stack (local variable). Just to be clear, the corrected version that @costaparas provided is clearly right answer, and my answer is just a minor commentary on the malloc version (2nd example).

Practise problem on C function about fixing errors

2 Answers2