tags:

views:

160

answers:

3

Inspired by this topic, I decided to write a simple program that does exactly that.
The logic isn't complicated and I have a working program 75% of the time.. The amount of asked numbers is defined as #define BUFSIZE x, where x can be an arbitrary int.
The problem arises when ((BUFSIZE+1) % sizeof(int)) == 0.

So for example, if BUFSIZE=10, my program behaves correctly, when BUFSIZE=11 I get odd behaviour.

Here is the sourcecode:

#include <stdio.h>
#include <stdlib.h>
#define BUFSIZE 7

int max(int *buf);

int main()
{
    int bufsize = BUFSIZE, *buf = malloc(sizeof(int[bufsize]));

    // read values
    int *ptr = buf;
    while(--bufsize + 1)
    {
        printf("Input %d: ", BUFSIZE - bufsize);
        scanf("%d", ptr);
        ++ptr;
    }

    // reset pointer and determine max
    ptr = buf;
    printf("\nMax: %d\n", max(ptr));
    // cleanup
    free(buf);
    ptr = NULL;
    buf = NULL;

    exit(EXIT_SUCCESS);
}

int max(int *buf)
{
    int max = 0;
    while(*buf)
    {
        printf("%d\n", *buf);
        if(*buf > max) max = *buf;
        ++buf;
    }
    return max;
}

And some sample output for BUFSIZE=2 (correct) and BUFSIZE=3 (incorrect).

suze:/home/born05/htdocs/experiments/c# gcc input.c && ./a.out
Input 1: 12
Input 2: 23
12
23

Max: 23

suze:/home/born05/htdocs/experiments/c# gcc input.c && ./a.out
Input 1: 12
Input 2: 23
Input 3: 34
12
23
34
135153

Max: 135153

I have the feeling it is something extremely logical but I can't put my finger on the exact cause of this misbehaviour. Could someone point out the (perhaps obvious) flaw to me?

+2  A: 

This

int bufsize = BUFSIZE, *buf = malloc(sizeof(int[bufsize]));

should be

int bufsize = BUFSIZE, *buf = malloc(sizeof(int[BUFSIZE + 1]));
buf[BUFSIZE] = 0;

In your current code you allocate memory for one integer (sizeof(int[bufsize]) evaluates to sizeof(int*)), instead you need memory for BUFSIZE integers and one extra integer containing null afterwards.

In your current code you have so-called undefined behavior - you use memory not legally allocated to you. In your case it sometimes works sometimes not. Well, at least you know it right now, not when it is pushes to control a nuclear power plant.

sharptooth
+4  A: 

It's actually pure luck that this even works for any values of BUFSIZE. (In fact, for me, it breaks on BUFSIZE=2). Here's why -- this:

while(*buf)

Is not an appropriate way to check for the end of your buffer. What this does is load the value at the address pointed to by buf and see if the contents are zero. Since you're never explicitly putting a zero at the end of your buffer, that's never necessarily going to be true, and that loop could run potentially forever, reading into memory that is past the end of your buf array and invoking undefined behavior.

You either need to allocate an extra element at the end of the buf array and set it to zero (but then your program won't work right if the user enters 0 as input), or explicitly pass the size of buf to the max function and use that to determine when you should stop looping.

Tyler McHenry
So, the way I iterate over my array isn't correct. What would be an appropriate way to do this? Would it really be necessary to pass a length to `max()`?
Dennis Haarbrink
Yes, I edited that in. The best way to do it is probably to pass the buf length to max and then run a `for (int i = 0; i < buf_length; ++i)` loop and within it access the elements in `buf` as `buf[i]`.
Tyler McHenry
Allright, I changed the prototype of `max()`, works like a charm now. It's hard getting used to the explicitness of these kinds of things (when coming from PHP world).
Dennis Haarbrink
C is a lower-level language, so on the one hand that does mean you need to be explicit, but on the other hand it means there's no magic going on behind the scenes, so you can always think through a problem by working through the code by hand as if you were the computer, since the code corresponds very directly to what the computer is actually doing.
Tyler McHenry
+1  A: 

You're treating buf as if it was a null terminated string array. You could do that if your data values are guaranteed never to be zero and you actually put the zero there (which your program isn't doing).

Rather try changing your max() function to something like this (adjusting prototype and calling location accordingly):

int max(int *buf, int count)
{
    int max = 0;

    // Check inputs
    if (buf == NULL || count <= 0)
    {
        printf("max(): bad parameter(s)\n");
        return 0;
    }

    while(count--)
    {
        printf("%d\n", *buf);
        if(*buf > max) max = *buf;
        ++buf;
    }
    return max;
}
Amardeep
Thanks, I really need to get it in my system to check my inputs!
Dennis Haarbrink