Invalid read of size 4 on fscanf

Question

Running my program through valgrind, I'm getting an invalid read of size 4 error in the following code (I think at the line where fscanf is called)

Important Info: numIntegers is the maximum number of integers that can be read, while numsInFile specifies the amount of integers present in the file.

I use the minimum of the two to initialize the array. The reason for this is because if numIntegers is 100 and there are 22 integers in the file, I only want an array of size 22. If numIntegers is 22 and there are 100 integers in the file, I still want an array of size 22 since only 22 can be read.

Here is my code:

// Get number of integers (first number)
int numsInFile;
fscanf(fp, "%d", &numsInFile);

// find minimum of numstoread and numintegers and use that as counttouse
    if(numsInFile < numIntegers)
        countToUse = numsInFile;
    else
        countToUse = numIntegers;

// Initialize the integers array with the minimum value as well - since it will only store this many
integers = malloc(sizeof(int) * countToUse);

for(int i = 0; i < numsInFile; i++){
    if(i < numIntegers){
        int currInt;
        if(fscanf(fp, "%d", &currInt) == 1)
            integers[i] = currInt;
        else
            break;
    }
    else
        break;
}

Any help is appreciated ahead of time. I can't for the life of me figure out why I'm getting this error in valgrind...

Code edited - No more errors

// Get number of integers (first number)
int numsInFile;
if( fscanf(fp, "%d", &numsInFile) != 1)
    fprintf(stderr, "Error reading number of integers(first number) from file. Exiting.\n");
    exit(1);
}

// find minimum of numstoread and numintegers and use that as counttouse
    if(numsInFile < numIntegers)
        countToUse = numsInFile;
    else
        countToUse = numIntegers;

// Initialize the integers array with the minimum value as well - since it will only store this many
integers = malloc(sizeof(int) * countToUse);

for(int i = 0; i < countToUse; i++){
    int currInt;
    if(fscanf(fp, "%d", &currInt) == 1)
        integers[i] = currInt;
    else{
        fprintf(stderr, "Error loading integer array in countAndSort(). Exiting.\n");
        exit(1);
}

You don't show the declaration of `numsInFile`. And [don't cast `malloc()`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Iharob Al Asimi, Feb 11 '15 at 21:38
@iharob Not sure what you mean by ignoring it, I'm checking if it's 1 before attempting to add the integer to the array so that I'm not adding garbage — JayB, Feb 11 '15 at 21:41
@JayB follow the `don't cast malloc()` link to see the answer to that question. And you *are* ignoring the return value from the first `fscanf()` call (outside the loop). — Paul Roub, Feb 11 '15 at 21:44
@iharob Ah, yes. I forgot about that. I'll add a check there — JayB, Feb 11 '15 at 21:46
@JayB forgetting that will make your program invoke undefined behavior, in such situation it's quite dificult to debug and fix your program. — Iharob Al Asimi, Feb 11 '15 at 21:48
Compile your program with optimization disabled, and run that under valgrind. Valgrind should tell you exactly on which line the access violation occurs. No need to guess (or make us do so). — John Bollinger, Feb 11 '15 at 21:49
@JohnBollinger yes with `gcc -Wall -Wextra -Werror -g3 -O0` in combination with `valgrind` it's possible to catch the most stupid mistakes instantly. — Iharob Al Asimi, Feb 11 '15 at 21:51
Unless the return value of `fscanf()` is checked, we are left to guess that a correct conversion occurred. Recommend to add code to check `fscanf()` return value in `fscanf(fp, "%d", &numsInFile);`. — chux - Reinstate Monica, Feb 11 '15 at 22:06
Are you by chance printing the integers somewhere? If you are, please post that code too. — Iharob Al Asimi, Feb 11 '15 at 22:06
@chux that could be the invalid read of size 4, I thought of it initially and the OP commented in my answer that he was going to change it. — Iharob Al Asimi, Feb 11 '15 at 22:07
Are you sure valgrind is complaining about this code at all? I don't see anything in it to explain the error. I especially don't see anything that could explain an invalid *read*. — John Bollinger, Feb 11 '15 at 22:08
@iharob, no. even if the `fscanf()` fails, subsequently accessing the value of the variable that was supposed to be read is not an "invalid read" in the valgrind sense. The value may not be well defined, but the storage location certainly is fine. — John Bollinger, Feb 11 '15 at 22:11
@John Bollinger Areas to explore: `numsInFile` value is unknown after the `fscanf()`. Could be negative. `numIntegers` is unknown - could be negative. `integers` is not known, could be `NULL`. — chux - Reinstate Monica, Feb 11 '15 at 22:12
@JohnBollinger I've fixed it all, but found another error in the allocation of space for a dynamic 2d array and I've added it to my question, I'd appreciate it if someone could take a look.. Thanks for everyone's help so far — JayB, Feb 11 '15 at 22:18
@chux, you are correct on all counts. The fact remains, however, that the code presented performs no reads via any pointer. Valgrind "invalid read" errors indicate reads of memory that valgrind does not think belong to the program, and you cannot get that from reading ordinary variables. (Correction: `fscanf()` reads the format strings, but reading via a pointer to a string literal should never generate an invalid read.) — John Bollinger, Feb 11 '15 at 22:19
@JayB, if you have a different problem, ask a different question. When you do, be specific about exactly what line valgrind complains about. And for what it's worth, it is doubtful that your new error is in the new code you posted. — John Bollinger, Feb 11 '15 at 22:29
@John Bollinger Your assessment sounds good. I take it if `integers` was `NULL`, a different error would be reported with `integers[i] = currInt;`? — chux - Reinstate Monica, Feb 11 '15 at 22:34
@chux, if `integers` were `NULL`, then assignment to `integers[i]` could be expected to generate an invalid *write*, but not an invalid *read*. It would likely also produce a segfault, even under valgrind. — John Bollinger, Feb 11 '15 at 22:37
@JohnBollinger yes you are right again, what valgrind would warn about is an `Uninitialized Value`. — Iharob Al Asimi, Feb 12 '15 at 00:05

Iharob Al Asimi · Accepted Answer · 2015-02-11T22:02:15.103

You allocate space for countToUse integers

integers = (int*)malloc(sizeof(int) * countToUse);

and then iterate through numsInFile, and then check or numIntegers, this is wrong, and you don't check the return value of your first fscanf() or of your malloc().

The following will not make valgrind complain

/* Get number of integers (first number) */
int numsInFile;

if (fscanf(fp, "%d", &numsInFile) != 1)
    doSomethingAboutIt_ButDoNotContinue();
/* find minimum of numstoread and numintegers and use that as counttouse */
if (numsInFile < numIntegers)
    countToUse = numsInFile;
else
    countToUse = numIntegers;
/*
 * Initialize the integers array with the minimum value as well
 * since it will only store this many 
 */
integers = malloc(sizeof(int) * countToUse);
if (integers == NULL)
    doSomethingAboutIt_ButDoNotContinue();
for (int i = 0 ; i < numsInFile ; i++)
{
    if (i < countToUse) /* You allocated enough space for countToUse int's */
    {
        int currInt;

        if (fscanf(fp, "%d", &currInt) == 1)
            integers[i] = currInt;
        else
            break;
    }
    else
        break;
}

there is of course a better way of doing this, by making the loop from 0 to countToUse, but I just wanted to point out what was wrong with your code.

These tips should help you improve your skills, which as far as I can see, are good

Always check the returned values from functions for errors, for example malloc() returns NULL on failure, and you didn't check for it.

It's very unlikely that this will happen, but it's not impossible, so it is wise to check.
Use compiler warnings, they help you with those small mistakes that are dificult to see normally.
When using valgrind compile your program with debugging info and without optimization, as pointed out by @JohnBollinger that will help you know at what exact line the problem is.

This is a typical compiler command that I use
```
gcc -Wall -Wextra -Werror -O0 -g3
```
when debugging it's an excelent way of preventing stupid mistakes, you can also add std=c99 if you care about portability and -pedantic if you really want to purify your code.

I don't see any good reason for the downvote, and yesterday I recieved my first reputation reversal for serial downvoting, which I assume is happening again. — Iharob Al Asimi, Feb 11 '15 at 21:49
@JayB Someone is downvoting all my posts no matter what, but don't worry it will be reverted within 24hrs. Did you fix your issue? — Iharob Al Asimi, Feb 11 '15 at 21:52
Not my downvote, but note that the original code does not appear to exceed the bounds of `integers`. The loop is structured oddly, but it never access an element of `integers` having index greater than or equal to the lesser of `numsInFile` and `numIntegers`. — John Bollinger, Feb 11 '15 at 22:00
@JohnBollinger you are absolutely right, I didn't pay attention to the check performed before, still it's wierd to use one variable for the size of the `malloc()`ed space and another variable to limit the read count... — Iharob Al Asimi, Feb 11 '15 at 22:03

Invalid read of size 4 on fscanf

Code edited - No more errors

1 Answers1