Learning C, need some help

[Richard Cranium]

Trying to implement a simple rrd. Code below. When calling rrd_write,
8 successful times in a row, the 9th one causes this to happen:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000001
0x00001f4f in rrd_write (rrd=0x3000f0) at rrd.c:81
81 rrd->value[rrd->position] = va_arg(ap, int);

I washed it through GDB a few times, and notice that starting from
rrd->value[8] I'm seeing strange memory addresses like 0xa and 0x1;
however, the calls to malloc() are clearly not failing.

I'm not sure what I'm doing wrong here.

The code:

--- cut here ---
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <errno.h>

typedef struct rrd
{
short position; /* Last-used position */
short length; /* Length of RRD */
short width; /* Width of RRD */
uint32_t **value; /* Value */
} rrd_t;

/* The following functions work with in-memory structures */
rrd_t *rrd_create(short length, short width);
int rrd_write(rrd_t *rrd, ...);

int main(int argc, char **argv)
{
rrd_t *rrd;

rrd = rrd_create(30, 2);

rrd_write(rrd, 1, 10);
rrd_write(rrd, 2, 10);
rrd_write(rrd, 3, 10);
rrd_write(rrd, 4, 10);
rrd_write(rrd, 5, 10);
rrd_write(rrd, 6, 10);
rrd_write(rrd, 7, 10);
rrd_write(rrd, 8, 10);
rrd_write(rrd, 9, 10);
rrd_write(rrd, 10, 10);
rrd_write(rrd, 11, 10);
rrd_write(rrd, 12, 10);
rrd_write(rrd, 13, 10);
rrd_write(rrd, 14, 10);
rrd_write(rrd, 15, 10);
rrd_write(rrd, 16, 10);
rrd_write(rrd, 17, 10);
rrd_write(rrd, 18, 10);
rrd_write(rrd, 19, 10);
rrd_write(rrd, 20, 10);
rrd_write(rrd, 21, 10);
rrd_write(rrd, 22, 10);
rrd_write(rrd, 23, 10);
rrd_write(rrd, 24, 10);
rrd_write(rrd, 25, 10);
rrd_write(rrd, 26, 10);
rrd_write(rrd, 27, 10);
rrd_write(rrd, 28, 10);
rrd_write(rrd, 29, 10);
rrd_write(rrd, 30, 10);
rrd_write(rrd, 1, 7);

return(1);
}

rrd_t *rrd_create(short length, short width)
{
rrd_t *rrd;
int i;

rrd = (struct rrd *)malloc(sizeof(struct rrd));

rrd->position = 0; /* Set the initial position to 0 */
rrd->length = length;
rrd->width = width;

/* Allocate enough memory for the entire rrd database */
rrd->value = malloc(length);

for (i = 0; i < length; i++)
{
rrd->value = malloc(width);
if (!rrd->value)
{
fprintf(stderr, "%s\n", strerror(errno));
exit(1);
}
}

return(rrd);
}

/* TODO: This needs some error checking */
int rrd_write(rrd_t *rrd, ...)
{
va_list ap;
va_start(ap, rrd);
int i;
static int j;

j++;

printf("J: %i\n", j);

// Write values
for (i = 0; i < rrd->width; i++)
{
rrd->value[rrd->position] = va_arg(ap, int);
}

// Check position
rrd->position++;
if (rrd->position > (rrd->length - 1))
{
rrd->position = 0;
}

va_end(ap);

return(1);
}

 

[Ben Pfaff]

Trying to implement a simple rrd.

What is an rrd?

Here are a couple of obvious problems:

typedef struct rrd
{
short position; /* Last-used position */
short length; /* Length of RRD */
short width; /* Width of RRD */
uint32_t **value; /* Value */
} rrd_t;
....

rrd->value = malloc(length);

rrd->value = malloc(length * sizeof *rrd->value);

for (i = 0; i < length; i++)
{
rrd->value = malloc(width);

rrd->value = malloc(width * sizeof *rrd->value);

if (!rrd->value)
{
fprintf(stderr, "%s\n", strerror(errno));
exit(1);
}
}

 

[Joachim Schmitz]

Trying to implement a simple rrd. Code below. When calling rrd_write,

Whatever rrd might be...

8 successful times in a row, the 9th one causes this to happen:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000001
0x00001f4f in rrd_write (rrd=0x3000f0) at rrd.c:81
81 rrd->value[rrd->position] = va_arg(ap, int);

I washed it through GDB a few times, and notice that starting from
rrd->value[8] I'm seeing strange memory addresses like 0xa and 0x1;
however, the calls to malloc() are clearly not failing.

I'm not sure what I'm doing wrong here.

The code:

--- cut here ---
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <errno.h>

typedef struct rrd
{
short position; /* Last-used position */
short length; /* Length of RRD */
short width; /* Width of RRD */
uint32_t **value; /* Value */

Where's that from? I find one in sdtint.h, which you did not #include

} rrd_t;

/* The following functions work with in-memory structures */
rrd_t *rrd_create(short length, short width);
int rrd_write(rrd_t *rrd, ...);

int main(int argc, char **argv)
{
rrd_t *rrd;

rrd = rrd_create(30, 2);

rrd_write(rrd, 1, 10);
rrd_write(rrd, 2, 10);
rrd_write(rrd, 3, 10);
rrd_write(rrd, 4, 10);
rrd_write(rrd, 5, 10);
rrd_write(rrd, 6, 10);
rrd_write(rrd, 7, 10);
rrd_write(rrd, 8, 10);
rrd_write(rrd, 9, 10);
rrd_write(rrd, 10, 10);
rrd_write(rrd, 11, 10);
rrd_write(rrd, 12, 10);
rrd_write(rrd, 13, 10);
rrd_write(rrd, 14, 10);
rrd_write(rrd, 15, 10);
rrd_write(rrd, 16, 10);
rrd_write(rrd, 17, 10);
rrd_write(rrd, 18, 10);
rrd_write(rrd, 19, 10);
rrd_write(rrd, 20, 10);
rrd_write(rrd, 21, 10);
rrd_write(rrd, 22, 10);
rrd_write(rrd, 23, 10);
rrd_write(rrd, 24, 10);
rrd_write(rrd, 25, 10);
rrd_write(rrd, 26, 10);
rrd_write(rrd, 27, 10);
rrd_write(rrd, 28, 10);
rrd_write(rrd, 29, 10);
rrd_write(rrd, 30, 10);
rrd_write(rrd, 1, 7);

return(1);
}

rrd_t *rrd_create(short length, short width)
{
rrd_t *rrd;
int i;

rrd = (struct rrd *)malloc(sizeof(struct rrd));

Drop the cast. Why using sizeof(struct rrd) rather than sizeof(rrd_t)?
Check for malloc's return value before writing to it.

rrd->position = 0; /* Set the initial position to 0 */
rrd->length = length;
rrd->width = width;

/* Allocate enough memory for the entire rrd database */
rrd->value = malloc(length);

Ah, no cast here, good. But check for return value. And allocate enough
rrd->value = malloc(length * sizeof *rrd->value);

for (i = 0; i < length; i++)
{
rrd->value = malloc(width);

Just like here! Again alloocate enough
rrd->value = malloc(width * sizeof *rrd->value);

if (!rrd->value)
{
fprintf(stderr, "%s\n", strerror(errno));

exit(1);
}
}

return(rrd);
}

/* TODO: This needs some error checking */
int rrd_write(rrd_t *rrd, ...)
{
va_list ap;
va_start(ap, rrd);
int i;
static int j;

j++;

This doesn't work in C89/C90, move va_start(ap, rrd);
down a couple of lines

printf("J: %i\n", j);

// Write values

Neither do these comments. Also they may break in the net

for (i = 0; i < rrd->width; i++)
{
rrd->value[rrd->position] = va_arg(ap, int);
}

// Check position
rrd->position++;
if (rrd->position > (rrd->length - 1))
{
rrd->position = 0;
}

va_end(ap);

return(1);
}

you malloc() but don't free(), relying that the end of main will sort it for
you, fair enough in this small thing, but otherwise bad practise.


Bye, Jojo

 

[Richard Cranium]

See below, and thanks, that seems to have fixed it up.

What is an rrd?

Round Robin Database. It's basically a method of storage for
time-series data which guarantees a constant storage size.

When using time-series data to compute aveages, the oldest value is
worthless to the average and can be discarded. So the array of values
is used written to on a round-robin basis. We use a position indicator
to tell us where in the array to go to next. The one-off from our
current position is always the oldest value. Once we hit the bottom of
the array, we start writing to the top of it again.

Thus storage size is always constant.

Real-time representation of this data (usually in graph form) is
typically of an average of set number of probes.

Historical data can be stored and computed in the same manner,
basically "Averages of Averages"

For example:

30 Second average: 1 probe, 1 second apart, average the data. Requires
a 30 * sizeof(value) storage space.

5 Minute average is 10 30 second averages, requires a 10* sizeof(value)
storage array

2 hour average is 24 5 minute averages, requires a 24*sizeof(value)
storage array

etc,
etc,
etc.

Here are a couple of obvious problems:

typedef struct rrd
{
short position; /* Last-used position */
short length; /* Length of RRD */
short width; /* Width of RRD */
uint32_t **value; /* Value */
} rrd_t;
...

rrd->value = malloc(length);

rrd->value = malloc(length * sizeof *rrd->value);

for (i = 0; i < length; i++)
{
rrd->value = malloc(width);

rrd->value = malloc(width * sizeof *rrd->value);

if (!rrd->value)
{
fprintf(stderr, "%s\n", strerror(errno));
exit(1);
}
}

 

[Flash Gordon]

Richard Cranium wrote, On 02/10/07 18:51:

Trying to implement a simple rrd. Code below. When calling rrd_write,
8 successful times in a row, the 9th one causes this to happen:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000001
0x00001f4f in rrd_write (rrd=0x3000f0) at rrd.c:81
81 rrd->value[rrd->position] = va_arg(ap, int);

I washed it through GDB a few times, and notice that starting from
rrd->value[8] I'm seeing strange memory addresses like 0xa and 0x1;
however, the calls to malloc() are clearly not failing.

If you are using Linux I suggest running your program through valgrind.
You should also look at what other tools are available for debugging
memory access problems.

I'm not sure what I'm doing wrong here.

The code:

--- cut here ---

You've missed the inclusion of stdint.h without which this will not
compile. If you retyped, then don't use copy and paste. If you used copy
and paste, then please check that you did not introduce any other errors.

You also need string.h for strerror.

#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <errno.h>

typedef struct rrd
{
short position; /* Last-used position */
short length; /* Length of RRD */
short width; /* Width of RRD */
uint32_t **value; /* Value */
} rrd_t;

rrd_t *rrd_create(short length, short width)
{
rrd_t *rrd;
int i;

rrd = (struct rrd *)malloc(sizeof(struct rrd));

Casting the result of malloc is rarely a good idea. Also there is a
generally far better way of determining what to pass to malloc. A lot of
people here recommend the following form for very good reasons:
rrd = malloc(sizeof *rrd);

In your program you should check that malloc succeeds. Otherwise if it
does fail the next line invokes undefined behaviour and is likely to
cause your program to crash. This applies to all the other malloc calls
as well.

rrd->position = 0; /* Set the initial position to 0 */
rrd->length = length;
rrd->width = width;

/* Allocate enough memory for the entire rrd database */
rrd->value = malloc(length);

Again, use
rrd->value = malloc(length * sizeof *rrd->value);
If you check you will probably find that sizeof *rrd->value is greater
than 1. You have allocated length bytes when you needed space for length
pointers.

As you can see, the patter often suggested protects you from this error.
It continues to protect you if you later change the type of rrd->value.

for (i = 0; i < length; i++)
{
rrd->value = malloc(width);

Same problem again.
rrd->value = malloc(length * sizeof *rrd->value);

if (!rrd->value)
{
fprintf(stderr, "%s\n", strerror(errno));
exit(1);

1 is not a portable value for exit, and on VMS will actually indicate
successful termination!
exit(EXIT_FAILURE);

}
}

return(rrd);
}

<snip>

 

[Mark McIntyre]

What is an rrd?

Golly.
See for example

http://oss.oetiker.ch/rrdtool/

any anywebsite displaying ntop stats
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan

 

[Richard Tobin]

What is an rrd?

Golly.
See for example

http://oss.oetiker.ch/rrdtool/

any anywebsite displaying ntop stats[/QUOTE]

What is ntop?

-- Richard

 

[Flash Gordon]

Richard Tobin wrote, On 03/10/07 12:18:

Golly.
See for example

http://oss.oetiker.ch/rrdtool/

any anywebsite displaying ntop stats

What is ntop?[/QUOTE]

Off topic ;-) Unless I miss my guess it is the first hit Google gives
for ntop.

The meaning of rrd is also off topic but had no bearing on the OPs problem.