Access to data into an array causes seg-fault

[Michel Rouzic]

I had a program that worked perfectly, and that read .wav files. I
changed something so the tags of the wave file, instead of being each
in a different variable, are all in an array, called tag. i also set
macros such as #define bitspersample tag[10] so when in my original
program it was written bitspersample now it acts like its tag[10], and
i also moved things that were in the main function into specific
functions. anyways, the program read the sound data into a multi
dimensionnal array.

now, since i did that change, when i try to access a value from that
multi-dimensional array by doing for example printf("%f",
sound[0][69]); in the main function, it causes a segmentation fault at
this precise line.

i think the multi-dimensionnal array is correctly allocated (the
difference between the old and new program is that the whole allocation
thing is in a function instead of the main function, and that it's
bases on values from that tag array to define its sizes instead of
regular integer variables)

now, i can access to a value of this multidimensional array, inside the
function that fills it, and also in the function that calls that
function and that allocates the array before calling it, but not back
in the main function.

is it a problem due to declaring such a "double **sound" array in the
main function and doing the malloc's in another function?

 

[Lawrence Kirby]

On Sun, 03 Jul 2005 06:52:45 -0700, Michel Rouzic wrote:

....

now, i can access to a value of this multidimensional array, inside the
function that fills it, and also in the function that calls that
function and that allocates the array before calling it, but not back
in the main function.

is it a problem due to declaring such a "double **sound" array in the
main function and doing the malloc's in another function?

There's nothing wrong with that as long as sound is set correctly to point
to the memory allocated in the called funciton.

It sounds like there is a problem with your particualr code but we can't
tell what that might be without seeing it.

Lawrence

 

[John Bode]

is it a problem due to declaring such a "double **sound" array in the
main function and doing the malloc's in another function?

How are you returning the array to main? Are you passing it as a
parameter to the allocating function? If so, are you passing it as
double **sound or double ***sound?

 

[Michel Rouzic]

How are you returning the array to main? Are you passing it as a
parameter to the allocating function? If so, are you passing it as
double **sound or double ***sound?

well, in main(), i declare it as double **sound, then call my
allocating function by wav_in(double **sound, ...) and inside that
wav_in function i allocate by doing this :

sound=(double **) malloc (channels * sizeof(double *));
for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

then i call a function called in_32(double **sound,...) which fills
that array, and then well the array returns by the same way it came in,
the problem is that back to main() i cant access values of my array

 

[Michel Rouzic]

On Sun, 03 Jul 2005 06:52:45 -0700, Michel Rouzic wrote:

...


There's nothing wrong with that as long as sound is set correctly to point
to the memory allocated in the called funciton.

It sounds like there is a problem with your particualr code but we can't
tell what that might be without seeing it.

Lawrence

ok, im not sure if its very appropriate to paste code here, but here it
is anyways (i made it minimalistic to show whats the problem, would'nt
have pasted the whole 16 kB code)

#include <stdio.h>
#include <stdint.h>
#include <math.h>
#define formattag tag[5]
#define channels tag[6]
#define samplespersec tag[7]
#define bitspersample tag[10]
#define datasize tag[12]
#define samplecount tag[13]

void in_32(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t i, ic;
float temp;

for (i=0;i<samplecount;i++)
for (ic=0;ic<channels;ic++)
{
fread(&temp, sizeof(float), 1, wavin);
sound[ic]=(double) temp;
}
}

void wav_in(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t ic;

samplecount=datasize/(bitspersample/8)/channels;

sound=(double **) malloc (channels * sizeof(double *));
for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

in_32(wavin, sound, tag);
fclose(wavin);
}

int main(int argc, char *argv[])
{
FILE *wavin;
wavin=fopen(argv[1], "rb");
if (wavin)
{
uint32_t tag[14];
double **sound;

wav_in(wavin, sound, tag);

printf("%f ", sound[0][69]); //that's where the seg fault happens
return 0;
}
}

 

[forayer]

Michel, this is were you went wrong. The 'sound' variable Lawrence Kirby
is refering to is the one declared in the main() function. See below.

void wav_in(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t ic;

samplecount=datasize/(bitspersample/8)/channels;

sound=(double **) malloc (channels * sizeof(double *));
for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

in_32(wavin, sound, tag);
fclose(wavin);

}

int main(int argc, char *argv[])
{
FILE *wavin;
wavin=fopen(argv[1], "rb");
if (wavin)
{
uint32_t tag[14];
double **snd;

Renamed 'sound' to 'snd' for ease of explantion.

wav_in(wavin, snd, tag);

printf("%f ", snd[0][69]); //that's where the seg fault happens
return 0;
}
}

The basic idea of passing a pointer to a function is to be able to
manipulate the _contents_ of the address pointed to by the pointer, not
the _pointer_ itself.

Suppose, 'snd' points to address location 1000. When you call the
wav_in() function, the 'sound' parameter is also set to point to address
location 1000. Within the wav_in() func when you allocate space and set
'sound' to point to it, 'snd' still points to 1000! Point to note: C has
only 'call by value' and no 'call by reference', i.e. any changes in
value to a passed variable within a function is not reflected in the
variable in caller's scope. Hope this makes sense. Maybe someone here
who has access to the standard can direct you to the appropriate section
(and direct me as to where I can download a copy of it :-] ).

Anyway, you can pass it as 'double ***sound' and do (*sound)=malloc()
and (*sound)[ic]=malloc().

Or, you can change the code as follows:
double **wav_in(FILE *wavin, uint32_t *tag)
{
double **sound;
/*
* rest of your code (incl. allocations)
*/
return sound;
}

and in main()
double **sound;
sound = wav_in(wavin, tag);


cheers,
forayer

 

[Chris Torek]

ok, im not sure if its very appropriate to paste code here ...

It is; especially something cut down to show just the problem, as
you did!

#include <stdio.h>
#include <stdint.h>
#include <math.h>
[snippage]

void wav_in(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t ic;

samplecount=datasize/(bitspersample/8)/channels;

sound=(double **) malloc (channels * sizeof(double *));

As someone else already noted, the most significant problem here is
that you pass in a value, which wav_in() copies to its local variable
named "sound"; then you overwrite the passed-in value with a new
value from malloc(); then you return, discarding the overwritten
local variable. The caller's version of the variable named "sound"
is not changed.

Another bug is that you used casts above, and here:

for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

You probably put in this cast because without it you got a message
of the form:

warning: assignment makes pointer from integer without cast

(more or less). But inserting the cast merely removes the warning,
without fixing the bug. The actual bug is forgetting to include
<stdlib.h>, which is how you tell the C compiler that malloc() has
"void *" as its return type. Without this, your C compiler would
see malloc() here for the first time and, in effect, think: "oh,
that must be a function that returns an int. I will just pretend
I saw:

int malloc();

before this point." Of course, this is the wrong declaration, and
if it works anyway, it is just by luck.

 

[Lawrence Kirby]

ok, im not sure if its very appropriate to paste code here, but here it
is anyways (i made it minimalistic to show whats the problem, would'nt
have pasted the whole 16 kB code)

Yes, posting minimal code is very appropriate.

#include <stdio.h>
#include <stdint.h>
#include <math.h>
#define formattag tag[5]
#define channels tag[6]
#define samplespersec tag[7]
#define bitspersample tag[10]
#define datasize tag[12]
#define samplecount tag[13]

void in_32(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t i, ic;
float temp;

for (i=0;i<samplecount;i++)
for (ic=0;ic<channels;ic++)
{
fread(&temp, sizeof(float), 1, wavin);
sound[ic]=(double) temp;
}
}

First note what you are doing with sound here. It is a value passed into
the function which is used to access an array. That's fine.

void wav_in(FILE *wavin, double **sound, uint32_t *tag)
{
uint32_t ic;

samplecount=datasize/(bitspersample/8)/channels;

sound=(double **) malloc (channels * sizeof(double *));
for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

in_32(wavin, sound, tag);
fclose(wavin);
}

Here wav_in is also defined to take a double ** argument for sound.
However it discards the value passed in and sets the local sound variable
to a new value. C passes arguments "by value" which means that the called
function gets a copy of the value, and the parameter variable within it is
independent of the caller. So any changes you make to sound within wav_in
are not visible outside the function.

There is another problem with the code i.e. calling malloc() without a
valid declaration in scope. You can correct that by including the
appropriate header which is <stdlib.h> in this case. You made the mistake
of casting the return value of malloc(). If you hadn't done this the
compiler would have warned you about the problem.

int main(int argc, char *argv[])
{
FILE *wavin;
wavin=fopen(argv[1], "rb");
if (wavin)
{
uint32_t tag[14];

double **sound;

wav_in(wavin, sound, tag);

These 2 lines can never be correct. You have defined an uninitialised
variable sound and then you pass the value of this uninitialised variable
to wav_in(). Acessing the value of an uninitialised variable is always an
error, although the effects tend to be nastier when the variable is a
pointer. What you need is code that updates the value of sound in main()
from wav_in. The most natural way is using a return value from the
function:

double **sound;

sound = wav_in(wavin, tag);

which you could write simply as

double **sound = wav_in(wavin, tag);

and then in wav_in make sound a simple local variable and return it.

You can also update the value of an object in the caller by passing a
pointer to it:

void wav_in(FILE *wavin, double ***soundret, uint32_t *tag)
{
uint32_t ic;
double **sound;

samplecount=datasize/(bitspersample/8)/channels;

sound = malloc (channels * sizeof *sound);
for (ic=0;ic<channels;ic++)
sound[ic] = malloc (samplecount * sizeof *sound[ic]);

in_32(wavin, sound, tag);
fclose(wavin);

*soundret = sound;
}

This uses a pointer to pointer to pointer to double (double ***) but
there's no need to worry about that, it is simply a type that can point at
a pointer to pointer to double (double **). In the caller you would use:

double **sound;

wav_in(wavin, &sound, tag);

printf("%f ", sound[0][69]); //that's where the seg fault happens

In your original code sound is still uninitialised at this point so
accessing its value as this code does is an error.

Lawrence

 

[John Bode]

How are you returning the array to main? Are you passing it as a
parameter to the allocating function? If so, are you passing it as
double **sound or double ***sound?

well, in main(), i declare it as double **sound, then call my
allocating function by wav_in(double **sound, ...) and inside that
wav_in function i allocate by doing this :

sound=(double **) malloc (channels * sizeof(double *));
for (ic=0;ic<channels;ic++)
sound[ic]=(double *) malloc (sizeof(double) * samplecount);

Here's your problem. Remember that in order to write to a parameter,
you must pass a pointer to that parameter. If you want to assign a
pointer value in your allocating function and have that change
reflected in main(), then you need to pass a pointer to the pointer.

In this specific case, you have a double** that you want to assign, so
you need to pass a pointer to it:

double **sound;
wav_in(&sound, ...);

void wav_in(double ***sound, ...)
{
...
*sound = malloc(sizeof (*sound)[0] * channels);
if (*sound)
{
for (ic=0; ic<channels; ic++)
{
(*sound)[ic] = malloc(sizeof (*sound)[ic][0] *
samplecount);
...
}
in_32(*sound,...);
}

Remember, don't cast the result of malloc(), and I've found it's a good
idea to use sizeof on the *object* you're allocating, rather than a
type. IOW, since we're allocating an array of T, use sizeof on an
array element, rather than sizeof T itself. Since the type of sound is
double ***, then the type of *sound is double **, the type of
(*sound)[0] is double*, and the type of (*sound)[0][0] is double, so

malloc(sizeof (*sound)[0] * N)

will allocate N elements of type double*, and

malloc(sizeof (*sound)[0][0] * N)

will allocate N elements of type double.

then i call a function called in_32(double **sound,...) which fills
that array, and then well the array returns by the same way it came in,
the problem is that back to main() i cant access values of my array

The call to in_32 works because you aren't trying to modify the value
of sound itself, just the array elements that sound points to.

Again, remember that if you want to write to a parameter in a function
and have that change reflected in the calling function, you have to
pass a pointer to that parameter.

Personally, when I write allocator functions, I return the thing being
allocated as the function return value. IOW, I'd write it like this:

double **newSoundArray(...)
{
double **sound;
...
sound = malloc(sizeof sound[0] * channels);
if (sound)
{
for (ic = 0; ic < channels; ic++)
{
sound[ic] = malloc (sizeof sound[ic][0] * samplecount);
...

return sound;
}

int main(void)
{
double **sound;

sound = newSoundArray(...);
...
}

Less dereferencing hell this way.