C program doesn't work all the times i run it

Question

I have got the following code and it doesn't work fine all the time I run it:

     ->> ./a.out gnl1_2.txt
1234567
abcdefg
     ->> ./a.out gnl1_2.txt
1234567
abcdefg
     ->> ./a.out gnl1_2.txt
1234567

It returned only the first line and sometimes gives me nothing (I might have some leaks). Basically it should give me the next line from a file in the line parameter (godlike = structure that has a string and a file descriptor)

int get_next_line(const int fd, char **line)
{
    static t_gnl *godlike;
    int i;

    i = 0;
    if (fd < 0 || !line || !fd)
        return (-1);
    if (!godlike)
        godlike = malloc(sizeof *godlike);
    //printf("%s\n", godlike[0].xx);
    while ((godlike[i].xx) && (godlike[i].fd != fd))
        i++;
    if (!godlike[i].xx)
    {
        godlike[i].fd = fd;
        godlike[i].xx = read_f(fd);
        if (godlike[i].xx == NULL)
            return (-1);
    }
    *line = ft_strcdup(godlike[i].xx, '\n');
    while ((godlike[i].xx[0]) && (godlike[i].xx[0] != '\n'))
    {
        godlike[i].xx[0] = '\0';
        godlike[i].xx++;
    }
    if (godlike[i].xx[0] == '\n')
    {
        godlike[i].xx[0] = '\0';
        godlike[i].xx++;
        return (1);
    }
    else
        return (0);
}

At the very least, include the definition of your t_gnl structure and some comments what you think the lines are doing... (also note that I don't see any explicit leaks here, possibly OOB behavior, and definitely you appear to be making assumptions that malloc will zero new memory which I'm pretty sure are not valid in all cases; see http://stackoverflow.com/questions/1622196/malloc-zeroing-out-memory) — Foon, Feb 26 '17 at 12:35
@Foon: `godlike`, at least the way the OP _wants_ to use it would constitute a runtime memory leak (ie: keep appending to a local `static` variable that allocates to the heap). But the real problem here is UB — Elias Van Ootegem, Feb 26 '17 at 12:40

Elias Van Ootegem · Accepted Answer · 2017-02-26T12:43:21.113

There's some undefined behaviour in this one:

godlike = malloc(sizeof *godlike);
while ((godlike[i].xx) && (godlike[i].fd != fd))
    i++;

You are allocating memory to hold 1 t_gnl object. To start with, the condition in your while loop evaluates to:

while((godlike[0].xx) && (godlike[0].fd != fd))

Which is perfectly valid, and equivalent to:

while(godlike->xx && godlike->fd != fd)

However, because the allocated memory is never initialized, this condition could hold true, especially when know that godlike has static storage. Each time the function is called, there's a chance your while condition holds true, and you end up evaluating something like

while((godlike[1].xx) && (godlike[1].fd != fd))

Which is accessing memory out of bounds.

You appear to be using your godlike pointer as an array, which it isn't. It's just 1 object, allocated on heap. If you really want to append to that memory, and have the data you store grow over time, then you ought to consider using a linked list, or at the very least: realloc the memory every time you add an element.

The main problem with doing that using the godlike pointer you have is that it's static, and function scoped at the same time. You'll alocate it, but you can only access it within that function, and that function is never free'ing the memory. That means that the memory you allocate will remain allocated for as long as the application runs (effectively behave like a runtime memory leak). If you use a list, or use realloc, the memory usage will grow every time the function is called, and memory won't get freed until your program exits. Any tool that analyses code like valgrind will flag this function as a potential memory leak for this reason.

score 0 · Answer 2 · answered Feb 26 '17 at 12:37

You have major problems in your code:

godlike is allocated to point to a single structure t_gnl allocated by malloc() and left uninitialized.
you access members that have not been initialized
once you increment i, you access elements beyond the first that have not been allocated.

As posted, the code has undefined behavior.

C program doesn't work all the times i run it

2 Answers2