Segmentation fault when reading from a file

Question

I have the following function:

void read_file(char* path, char** data)
{
    FILE*   file = NULL;
    size_t  size, result = 0;

    *data = NULL;
    file = fopen(path, "rb");

    if (file == NULL)// error opening file
    {
        return;
    }

    fseek(file, 0, SEEK_END);
    size = ftell(file) + 1;
    rewind(file);

    *data = (char*)malloc(size);
    if(*data == NULL)
        return;

    result = fread(*data, 1, size, file);
    if (result != size - 1)// error reding file
    {
        *data = NULL;
    }
    printf("LINE=%u\n", __LINE__);
    (*data)[size-1] = '\0';

    printf("LINE=%u\n", __LINE__);
    fclose(file);
    return;
}

I am getting a Segmentation fault on the line right in between the two printf("LINE=%u\n", __LINE__); statements. I don't understand why this is. When I'm looking at this line, it seems (*data) would have a type of (char *) which should certainly be able to be used with the index operator [].

What am I missing?

Did you output the value of size in your debugger? In particular, check that it isn't 0. — Lightness Races in Orbit
– Lightness Races in Orbit, Commented Oct 5, 2011 at 17:22
Probably, but you're not compiling it in both are you? If nothing else, this is not idiomatic C++ code. Not by a long stretch. Even when strictly valid, it would be considered C rather than C++ and would not pass C++ code review. — Lightness Races in Orbit
– Lightness Races in Orbit, Commented Oct 5, 2011 at 17:22
Actually, it's giving 4096 as the size, which is certainly not correct. — Adam S
– Adam S, Commented Oct 5, 2011 at 17:24

Paul R · Accepted Answer · 2011-10-05 17:24:12Z

5

Probably the if (result != size - 1) test is failing and then you reset *data to NULL (which is a memory leak, BTW), and then you try to write to (*data)[size-1] - oops !

answered Oct 5, 2011 at 17:24

Paul R

214k38 gold badges402 silver badges579 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

AndersK · Accepted Answer · 2011-10-05 17:32:48Z

1

some pointers:

ftell returns -1 on failure, so if that is the case this will be 0 size = ftell(file) + 1;

size_t on some platforms is unsigned int, it may be good to have that in mind.

doing *data = NULL; is not a good idea, free it first free( *data );

put some if statements in your code to catch errors, don't assume everything will work e.g. assert( size>0 );

answered Oct 5, 2011 at 17:32

AndersK

36.1k6 gold badges64 silver badges86 bronze badges

Comments

RushPL · Accepted Answer · 2011-10-05 17:39:57Z

I have tested your code and it works for me - I have added returning of file's size to properly pass the data to fwrite.

> ./a.out arm-2010.09-good.tar.bz2 | sha1sum && sha1sum arm-2010.09-good.tar.bz2 
alloc size of 37265592
6bdff517bcdd1d279fc84ab3a5fbbca34211a87c  -
6bdff517bcdd1d279fc84ab3a5fbbca34211a87c  arm-2010.09-good.tar.bz2

furthermore Valgrind reports no warning and errors so .. loooks OK!

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

size_t read_file(char* path, char** data)
{
    FILE*   file = NULL;
    size_t  size, result = 0;

    *data = NULL;
    file = fopen(path, "rb");

    if (file == NULL)// error opening file
    {
        return 0;
    }

    fseek(file, 0, SEEK_END);
    size = ftell(file) + 1;
    rewind(file);

    fprintf(stderr, "alloc size of %i\n", size);
    *data = (char*)malloc(size);
    if(*data == NULL)
        return 0;

    result = fread(*data, 1, size, file);
    if (result != size - 1)// error reding file
        *data = NULL;
    (*data)[size-1] = '\0';
    size--; // report file size

    fclose(file);
    return size;
}

int main(int argc, char** argv)
{
    char* data;
    if(argc<2)
        return 0;
    size_t siz = read_file(argv[1], &data);
    if(data) {
        fwrite(data, 1, siz, stdout);
        free(data);
    }
    else {
        fprintf(stderr, "No data returned\n");
    }
    return 0;
}

John Bode · Accepted Answer · 2011-10-05 18:40:28Z

0

Here's the probable source of the problem:

if (result != size - 1)// error reding file
{
    *data = NULL;
}
printf("LINE=%u\n", __LINE__);
(*data)[size-1] = '\0';

What happens if there is an error reading the file? You set *data to NULL, and then immediately try to dereference it - bad juju.

Note that this also results in a memory leak; you don't free the memory that *data points to.

Restructure your code so that (*data)[size-1] = '\0' is executed only if the read operation was successful:

if (result != size - 1)
{
  free(*data);
  *data = NULL;
}
else                    
{                       
  (*data)[size-1] = 0;  
}

answered Oct 5, 2011 at 18:40

John Bode

125k19 gold badges130 silver badges211 bronze badges

Collectives™ on Stack Overflow

Segmentation fault when reading from a file

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related