0

I have such a function that reads all the contents of the data file.txt, which contains variables of the given structure type, in the format DD mm yyyy. this is the function

void readFromFile(struct data *element){

    FILE *data_file;
    data_file = fopen("D:\\univer\\sem2\\tehProg\\struct_and_files\\struct.txt","rw");
    if(data_file == NULL){
        fprintf(stderr, "Nu se poate de deschis fisierul\n");
        return;
    }
    char line[100];


    while (fgets(line, sizeof line, data_file))
    {
        int y, m, d;
        if (sscanf(line, "%d %d %d", &d,&m,&y))
        {
            element[n].d = d;
            element[n].m = m;
            element[n].y = y;
            ++n;
        }
    }

    fclose(data_file);
}

For this, I assign memory to the variable DD1, and transmit it as a paramatron in my function, from main.

struct data *dd1 = malloc(sizeof *dd1);
            readFromFile(dd1);
            displayN(dd1);
            struct data maximum = checkMax(dd1);
            struct data minimum = checkMin(dd1);

            printf("\n Data maxima %d %d %d",maximum.d,maximum.m,maximum.y);
            printf("\n Data minima %d %d %d", minimum.d,minimum.m,minimum.y);
            printf("\n Nr de ani bisecti :  %d ", checkLeapYear(maximum,minimum));
            free(dd1);

Everything works, but I get this error code (-1073741819 (0xC0000005)) in the console. What could be the cause?

kammy
  • 93
  • 8
  • 2
    `malloc(sizeof *dd1)` -- this will give you space to hold a single element. Multiply with a reasonable max size and enforce it when you read the data. (Alternatively, change the `while` in your reading fuction to `if`. `:)`) – M Oehm May 02 '22 at 13:52
  • 3
    `++n;` but where is `n` defined and initilaised? Perhaps elsewhere, but the function never checks for array overrun. – Weather Vane May 02 '22 at 13:52
  • 2
    The line `struct data *dd1 = malloc(sizeof *dd1);` allocate memory for only one `data` object, but your function attempts to write more than one. – Adrian Mole May 02 '22 at 13:52
  • Aside: `if (sscanf(line, "%d %d %d", &d,&m,&y))` is true when `EOF` is returned. It should be `if (sscanf(line, "%d %d %d", &d,&m,&y) == 3)` – Weather Vane May 02 '22 at 13:55
  • @WeatherVane the point is that I never know what the number of records in the file is. n is declared global and initialized with 0 – kammy May 02 '22 at 13:55
  • In that case you must not blindly read on, but reallocate the array. Even more reason to keep a check on `n`. – Weather Vane May 02 '22 at 13:56
  • Further aside: `if( sscanf(...) )` will also evaluate as true when `sscanf` only matches 1 or 2 conversion specifiers, and if that happens on the first iteration of the loop then attempting to assign the unwritten variables is undefined behavior. – William Pursell May 02 '22 at 13:57
  • @WeatherVane i trien this element = realloc(element, n + 1); but nothing has changed – kammy May 02 '22 at 13:57
  • @WilliamPursell i changed in if (sscanf(line, "%d %d %d", &d,&m,&y) == 3) – kammy May 02 '22 at 13:59
  • That too, fails to allocate the correct amount, and, the caller won't know about the altered pointer. You need a rethink on the strategy. – Weather Vane May 02 '22 at 13:59
  • @WeatherVane And what to do in this case? – kammy May 02 '22 at 14:01
  • You have not taken into account `sizeof` anything. – Weather Vane May 02 '22 at 14:01
  • Sounds like you need to read up on memory allocation and how it works. – Robert Harvey May 02 '22 at 14:01
  • @WeatherVane I understand, I should multiply that thing by an n, but since I do not know the nuamrul of elements, I do not understand what to do – kammy May 02 '22 at 14:03
  • @kammy The simplest approach is to allocate some space and grow it as needed. eg, initially allocate `128 * sizeof *dd1`. Set `cap = 128`. Each time you read 128 items, realloc to `cap += 128` items. – William Pursell May 02 '22 at 14:22
  • One thing you could do, to make the allocation easier, is to parse the file twice. Once to find the size, then to read data in the allocated array. – Weather Vane May 02 '22 at 14:22
  • @WilliamPursell Did I understand you correctly ? I changed this struct data *dd1 = malloc(128*sizeof *dd1); in int main and this element = realloc(element, n + 128); in my function. Apparently, the problem is gone – kammy May 02 '22 at 14:38
  • @kammy You don't need to reallocate on each read. You can do that, and it is really easy to implement, but it's the worst possible approach. OTOH, if it works, use it. – William Pursell May 02 '22 at 14:44

1 Answers1

0
  • struct data *dd1 = malloc(sizeof *dd1); allocates room for a single struct. Yet in the function, you use this as if it was an array of structs: element[n]. So you access the array wildly out of bounds. Hence the access violation reported.

    In case you wish to use realloc inside the function to solve this, you need to either pass the struct array as pointer-to-pointer or return a pointer to it from the function.

  • if (sscanf(line, "%d %d %d", &d,&m,&y)) is wrong since sscanf will either return 1 to 3 in case 1-3 numbers were read successfully or EOF. It will not return NULL like fgets. Also you probably shouldn't have spaces in the sscanf call.

  • n ought to be a local variable inside the function, not an icky global.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • You may want to put a link to [this](https://stackoverflow.com/q/484635/1707353) behind the word "icky". – Jeff Holt May 02 '22 at 14:09
  • @JeffHolt In this particular case it is "icky" mainly because if you call the function multiple times, it will fail. But of course all other problems with tight coupling, namespace pollution, thread safety and so on are there. – Lundin May 02 '22 at 14:11
  • But the solution is not to make `n` local. Instead, define a container that tracks its capacity. – William Pursell May 02 '22 at 14:23
  • @WilliamPursell Just pass the size as parameter. `n` is just plain old local loop iterator, which could optionally be returned in case the caller is interested about it. – Lundin May 02 '22 at 14:33