Strncat returns garbage values

Question

I am learning GLFW and I am making a code to load shaders into my code. This is my code:

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


char* loadShader(const char* shaderName){
    FILE *shader = fopen(shaderName, "r");
    char c;
    char* realShader;
    while ((c = fgetc(shader))!= EOF){
        strncat(realShader, &c, sizeof(c));
    }
    return realShader;
}


int main(void){
    char* filename = "default";
    char* shader = loadShader(filename);
    printf("%s\n", shader);
    return 0;
}

I tested this with the following text file(i.e. default):

hello world
bye world

But this returns P a hello world aaa

Then I used another test file (default.frag):

#version 330 core
out vec4 FragColor;

void main(){
    FragColor = vec4(1.0f, 1.0f, 1.0f, 1.0f);
}

and this returns

P a#version 330 a

What am I doing wrong here?

`char* realShader;` Where does that point? You need to allocate some memory into which you concatenate your source string(s). And you need to read up on how `strncat` works - it take **nul-terminated strings** as input, not single characters. — Adrian Mole, Sep 19 '21 at 22:21
Reading a file character by character is needlessly complicated, a simpler solution can be found in e.g.: https://stackoverflow.com/questions/174531/how-to-read-the-content-of-a-file-to-a-string-in-c — UnholySheep, Sep 19 '21 at 22:24
Thanks @Adrian Mole. I tried char* realShader = malloc(sizeof(shader)); and it worked — The Parallax, Sep 19 '21 at 22:26
See: [Undefined, unspecified and implementation-defined behavior](https://stackoverflow.com/q/2397984/3422102) and [What is indeterminate behavior in C++ ? How is it different from undefined behavior?](https://stackoverflow.com/q/11240484/3422102) and [Undefined behavior](https://en.cppreference.com/w/c/language/behavior) — David C. Rankin, Sep 19 '21 at 23:25
Functions like `strncat`, `strncpy`, `snprintf` etc. almost always need a `size_t` argument indicating the amount of space remaining in the *destination* buffer. If you are relating it *not* to the destination buffer and *only* to the source buffer, this should be a good indication you might be doing something wrong. It is only when there's some kind of guarantee about the destination buffer size that the `n` functions can realistically safely "ignore" it. — Cheatah, Sep 20 '21 at 00:49

LSerni · Answer 1 · 2021-09-19T23:19:06.843

strncat was made for strings. But &c is not a string.

Also, the original buffer is never allocated, so its contents is undefined. You're lucky that the program didn't immediately crash.

Afer allocating a suitable buffer, you could use fgets here, or keep reading the characters in, one by one. In this latter case, remember to add a terminating zero, as this is the C standard for strings. If you use fgets, you'll read in also any line-terminating character ("\n"), and you might want to remove it.

Also, remember to close the files when you're through using them, or before exiting the function where the file pointer is defined - otherwise, system resources will leak.

For example:

FILE *shader;
char* realShader;

// Open the file, and check that opening succeeded.
if (NULL != (shader = fopen(shaderName, "r"))) {
    // Allocate some memory for realShader, and check
    // that the allocation did work.
    if (NULL != (realShader = malloc(1024))) {
        char *c;
        // Read one line
        if (NULL == fgets(realShader, 1024, shader)) {
            // Read failed. No sense in keeping memory allocated
            free(realShader); realShader = NULL;
        } else {
            // If there is a return at end of line...
            c = strchr(realShader, '\n');
            if (c) {
                // ...kill it.
                *c = 0;
            }
        }
    }
    fclose(shader); // shader = NULL;
}
return realShader;

UPDATE

If you want to read the whole file, then you need to know its size (unless you want to use realloc in a cycle, also doable). For that, you would use the stat function after temporarily allocating a buffer for the stat info:

// Create the structure on the stack as a temporary variable
// (no malloc)
struct stat statbuf;
fstat(shaderName, &statbuf);

// This is, in general, not a good idea because "st_size" is not
// necessarily what malloc expects. On my system it is a 64 bit
// value, and so is malloc's size_t, so things work. Otherwise a
// compiler warning is the least you can expect.
// (also, blindly loading large chunks of data into memory is
// not the first thing you should go for if at all avoidable).
realShader = malloc(statbuf.st_size);

@AdrianMole you were right in objecting to 'vague terms', but the thing is, I often find that if I just plug in some working code, there's a risk it will be blindly adopted - and maybe I did not consider all edge cases, or misunderstood, or it works in the scenario I thought but not in the real one. So I often prefer to "encourage" to read the manuals and understand what the code *does*. — LSerni, Sep 19 '21 at 23:24
Not a problem. I've been in similar situations ... it's sometimes a tricky call with 'homework' style questions. But, ultimately, we want good, complete answers on Stack Overflow, which stand the test of time. — Adrian Mole, Sep 19 '21 at 23:49

Strncat returns garbage values

1 Answers1