Program misbehaves 25% of the time

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?

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.

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.

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;
}