Pointer Based Data Structures

Question

So my homework instructs me to read in a .WAV file and fill out the header and data according to their respective endians. I believe I have malloced enough memory for my header and data struct, but when I run the program, I end up with a Segmentation Fault 11. I can't seem to find where I went wrong. Any help would be appreciated!

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

#include "prog9.h"

/*
 * little_endian_2 - reads 2 bytes of little endian and reorganizes it into big endian
 * INPUTS:       fptr - pointer to the wav file
 * OUTPUTS:      none
 * RETURNS:      the data that is converted to big endian
 */
int little_endian_2(FILE *fptr)
{
    int count;
    char temp[2];
    count = 0;
    while (count < 2)
    {
        fscanf (fptr, "%s", temp);
        count++;


    }

    char holder;

    holder = temp[1];
    temp[1] = temp[0];
    temp[0] = holder;

    printf ("%c", temp[0]);
    printf("%c", temp[1]);
    count = atoi(temp);
    return count;
}

/*
 * little_endian_4 - reads 4 bytes of little endian and reorganizes it into big endian
 * INPUTS:       fptr - pointer to the wav file
 * OUTPUTS:      none
 * RETURNS:      the data that is converted to big endian
 */
int little_endian_4(FILE *fptr)
{
    int count = 0;
    char temp[4];

    while (count < 4)
    {
        fscanf (fptr, "%c", temp);
        count++;

    }

    char holder;

    holder = temp[3];
    temp[3] = temp[0];
    temp[0] = holder;

    holder = temp[2];
    temp[2] = temp[1];
    temp[1] = holder;

    count = atoi(temp);
    return count;
}

/*
 * read_file  - read the wav file and fill out the wav file struct
 * INPUTS:      wavfile - a string that contains the name of the file
 * OUTPUTS:     none
 * RETURNS:     the pointer to the wav file struct created in this function
 * SIDE EFFECT: prints the information stored in the wav struct
 */
WAV *read_file(char *wavfile)
{
    WAV* wav_ptr = (WAV*)malloc(sizeof(WAV));

    FILE *fp;
    fp = fopen(wavfile,"r");

    fscanf (fp, "%c", wav_ptr->RIFF); //For RIFF

    wav_ptr->ChunkSize = little_endian_4(fp);   //For ChunkSize

    fscanf (fp, "%c", wav_ptr->WAVE); //For WAVE

    fscanf (fp, "%c", wav_ptr->fmt); //For fmt

    wav_ptr->Subchunk1Size = little_endian_4(fp); //for Subchunk1size

    wav_ptr->AudioFormat = little_endian_2(fp); //For audioformat

    wav_ptr->NumChan = little_endian_2(fp); //for numchan

    wav_ptr->SamplesPerSec = little_endian_4(fp); //for samp/sec

    wav_ptr->bytesPerSec = little_endian_4(fp); //for bytes/sec

    wav_ptr->blockAlign = little_endian_2(fp); //for block alight

    wav_ptr->bitsPerSample = little_endian_2(fp); //for bits/sample

    wav_ptr->extra = (char*)malloc((wav_ptr->Subchunk1Size - 16)*sizeof(char)); //allocate memory for extra

    fscanf (fp, "%c", wav_ptr->extra); //if extra

    fscanf (fp, "%c", wav_ptr->Subchunk2ID); // for subchunk2id

    wav_ptr->Subchunk2Size = little_endian_4(fp); //for subchunk 2

    wav_ptr->data = (short int*)malloc((wav_ptr->Subchunk2Size)*sizeof(short int)); //allocate memory for data

    fscanf (fp, "%hd", wav_ptr->data); //for data

    return wav_ptr;

Did you run your program in a debugger to see which line caused the segfault? — andlabs, Apr 12 '15 at 17:57
fscanf (fptr, "%s", temp); => this reads a null terminated string. and you do that twice. a unique fread(temp, 1, 2, fptr) should be enough to read two bytes. — philippe lhardy, Apr 12 '15 at 18:01
I'm still trying to learn valgrind by myself since our class hasn't taught the debugging tools yet so I can't say I've tried. — bearswithattitude, Apr 12 '15 at 18:21
Also, the instruction says that in the little endian 2 and 4, the bytes should be read in via fscanf. Is there anyway to read up to 2 bytes and 4 bytes using fscanf? — bearswithattitude, Apr 12 '15 at 18:22
Don't start with valgrind. Start with gdb. That's the general-purpose debugger. — andlabs, Apr 12 '15 at 18:24
Thanks for the quick reply. Do you know any good links where I can learn running gdb for my program? — bearswithattitude, Apr 12 '15 at 18:26
fscanf(fptr,"%2c",temp) will actualy read two chars, this is what you want, no need of loop.. — philippe lhardy, Apr 12 '15 at 18:53
man scanf and man scanf are the very same manual page, format specification is the same. read them in detail... — philippe lhardy, Apr 12 '15 at 19:14

score 2 · Answer 1 · edited May 23 '17 at 12:14

reads 2 bytes of little endian and reorganizes it into big endian

I can't imagine why you would want to do such a thing. Usually what we want to do is read two bytes and convert them to whichever endianness the machine uses. This works best if our code doesn't care what endianness is used by the machine. For example...

int little_endian_2(FILE *source, unsigned int *destination) {
    int x = getchar();
    if (x < 0) { return x; }
    *destination = x;

    x = getchar();
    if (x < 0) { return x; }
    *destination += (unsigned int) x * 256;
    return 0;
}

Note the intention of this code: Read two bytes and add them to *destination, using multiplication as necessary to move the bytes to their correct positions.

... and so we reach our first (and most likely) problem: fscanf (fptr, "%s", temp); does not read a single byte as you seem to think; it reads a string of bytes, up to the first whitespace character. That could be more than two bytes, so it might overflow your temp buffer.

A non-issue in this case, but you shouldn't cast malloc. Similarly, sizeof(char) is always 1, and multiplying by 1 is pointless. If you're concerned that someone might change the type of wav_ptr->extra, then you should probably write:

wav_ptr->extra = malloc((wav_ptr->Subchunk1Size - 16) * sizeof *wav_ptr->extra);

Don't forget to check for null pointers before you use malloc()'ed blocks of memory. Similarly, don't forget to check fp (after your fopen call) for a null pointer... In fact, you should ALWAYS check the return value, THAT INCLUDES FOR fscanf!

I notice that you wrote fscanf (fp, "%c", wav_ptr->RIFF);. This is fine, so long as wav_ptr->RIFF is an array of 1 character (e.g. char RIFF[1];). You need to make sure fscanf has pointers to everything it needs to modify (or else it can't modify them), so if wav_ptr->RIFF is a mere character this code should look like:

int x = fscanf (fp, "%c", &wav_ptr->RIFF);

Note the ampersand! Also, don't forget to check x. Similarly, unless wav_ptr->data is declared as short int data[1], you probably want an ampersand in this, so fscanf has a pointer to wav_ptr->data:

fscanf (fp, "%hd", wav_ptr->data);

Since we don't have a complete MCVE, this is about as far as I can speculate... However, I was able to identify some issues that will commonly cause segfaults. If you happen to amend your question with an MCVE and updated code, please ping me and I'll update my answer to correspond.

Pointer Based Data Structures

1 Answers1