0

So, valgrind gave me that error and I ran it with --track-origins=yes and found the line where the error is, but I don't understand what the error is or how to fix it. 

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

typedef struct Date{
    int year;
    int month;
    int day;
} Date;

typedef struct Data{
    Date date;
    float temp;
    float uncertainty;
    char country[100];
} Data;

int main(){
    FILE* f = fopen("tempcountries_short.csv", "r");
    char* line = NULL;
    int capacity = 0;
    int countries_capacity = 0;
    int line_ix = 0;
    char c;
    Data* country = NULL;
    while ((c = fgetc(f)) != '\n'){ 
        if (line_ix + 1 > capacity){
            if (capacity == 0)                    
                capacity = 10;
            else
                capacity = capacity * 2;
            line = realloc(line, capacity); 
        }
        line[line_ix] = c;
        line_ix++;
    }
    if (countries_capacity == 0)
        countries_capacity = 10;
    else
        countries_capacity = countries_capacity * 2;
    country = realloc(country, countries_capacity);
    printf("%i\n",sscanf(line, "%i - %i - %i, %f , %f , %s", 
           &country->date.year, &country->date.month,
           &country->date.day, &country->temp, &country->uncertainty,
           country->country));
}

This is the output from Valgrind, with options --leak-check=full and --track-origins=yes: https://pastebin.com/EyqDGBmQ As you can see there are many other errors, and I don't understand what causes them either.

The program is reading lines from a file with data about many countries temperatures, I just took the part of the code for a single country to replicate the error, but country is supposed to be an array of many Data structs. Here's an example line from the file I'm reading:

1972-03-01,4.787,0.342,Slovakia

chilliefiber
  • 571
  • 2
  • 7
  • 18

3 Answers3

3
country = realloc(country, countries_capacity);

You appear to be relying on realloc(0, new_size) to behave as malloc(new_size). That's fine, but then you must make sure that the pointer variable passed to realloc really is null. No code before this point initializes the country variable, and in its declaration ...

Data* country;

... there's no initializer. Change this to

Data *country = 0;

and that part of the problem should go away.

Often, when you get a whole string of errors from valgrind, only the first one is meaningful, so see if that fixes all of your problems.

EDIT: With the version of the program with the above corrected, fed the sample line of input shown, I do not get any complaints about uninitialized values inside realloc, but I do still get a complaint about uninitialized values inside sscanf:

==28542== Conditional jump or move depends on uninitialised value(s)
==28542==    at 0x4C340E6: rawmemchr (vg_replace_strmem.c:1409)
==28542==    by 0x4EB6291: _IO_str_init_static_internal (strops.c:41)
==28542==    by 0x4EA476C: __isoc99_vsscanf (isoc99_vsscanf.c:41)
==28542==    by 0x4EA46D3: __isoc99_sscanf (isoc99_sscanf.c:31)
==28542==    by 0x108886: main (in /tmp/a.out)

and a complaint about invalid writes:

==28542== Invalid write of size 4
==28542==    at 0x4E98FFB: _IO_vfscanf (vfscanf.c:1898)
==28542==    by 0x4EA4781: __isoc99_vsscanf (isoc99_vsscanf.c:43)
==28542==    by 0x4EA46D3: __isoc99_sscanf (isoc99_sscanf.c:31)
==28542==    by 0x108886: main (in /tmp/a.out)
==28542==  Address 0x51f41a8 is 8 bytes inside a block of size 10 alloc'd
==28542==    at 0x4C2CABF: malloc (vg_replace_malloc.c:298)
==28542==    by 0x4C2EE04: realloc (vg_replace_malloc.c:785)
==28542==    by 0x10883C: main (in /tmp/a.out)

These are both caused by real bugs: the first is because the line array is never made into a proper C-string (by adding a nul terminator), the second is because the size passed to realloc is wrong. You need

line[line_ix] = '\0';

immediately after the while loop, and

country = realloc(country, countries_capacity * sizeof(Data));

instead of the realloc call you have now.

With those changes, I get no valgrind complaints. You do still have the problem that you're using sscanf, which is always a bad idea, but valgrind can't help you with that.

zwol
  • 135,547
  • 38
  • 252
  • 361
  • Why not use bzero instead of = NULL or = 0 ? It is exactly the same ? – Antonin GAVREL May 17 '18 at 16:35
  • Actually that was a mistake I made when I copied the code to limit the mistake, in the original I had the initialization to NULL. I just changed it and ran Valgrind again, it didn't solve anything. Also, the link you gave says that scanf is a bad idea, not sscanf. – chilliefiber May 17 '18 at 16:40
  • Re the remaining valgrind complaints see edits. Re (s)scanf, all of the flaws described for scanf at the page in the link apply _equally_ to sscanf (yes, I know that the author of that page doesn't see it that way). – zwol May 17 '18 at 16:56
  • @AntoninGavrel `bzero` is deprecated and one should use `memset` in its place. And even then, the point is to set a range of bytes; if just setting one var, there isn't much reason to use such a sledgehammer. – Christian Gibbons May 17 '18 at 17:36
  • @zwol Can you explain why you think so about sscanf, perhaps in this question: https://stackoverflow.com/questions/5873402/is-sscanf-considered-safe-to-use? I'm genuinely curious. By the way, you are right, now everything runs correctly, so I accepted your answer. – chilliefiber May 17 '18 at 18:33
1

The problem is here:

country = realloc(country, countries_capacity);

country has not been initialized, it contains an indeterminate value.

Just initialize country to NULL upon declaration like this:

Data* country = NULL;
Jabberwocky
  • 48,281
  • 17
  • 65
  • 115
  • As I said in the other comment, that doesn't actually change anything, and in the original code I had the initialization to NULL. I've updated the code in the question. – chilliefiber May 17 '18 at 16:42
1

The last character of line must be set to \0, to be a proper null terminated string.

phd
  • 3,669
  • 1
  • 11
  • 12