CODEWITHSUNDEEP
cnewline
P.Lorand
P.Lorand

Reputation: 1432

C - Unable to read from a file that has no trailing new line in it

I am working on a program in C that has to read a line from a file descriptor. It has to return 1, 0 or -1 if there was an error but the line is read from main using the address of a pointer. The program fails to output anything when I want to read from a file that I have created like this: echo -n "test test" > filetest1 (-n do not output the trailing newline)

I use an array in which I store the new line until I find \n and then I allocate memory for a pointer and store that line in it. The program copies "test test" in the pointer but it does not output anything when I compile. And I don't understand why.

I am not allowed to :

Upvotes: 0

Views: 392

Answers (1)

Nominal Animal
Nominal Animal

Reputation: 39426

There is no need to use an intermediate buffer. Assuming the return value must be -1, 0, +1, the function prototype

int get_next_line(int fd, char **lineptr);

is deficient, because it means you cannot read content with embedded nul bytes ('\0'), and reusing the same dynamically allocated line is not really feasible. (Sure, you can use strlen() to guess how long the previous line might be, but it's just a guess.)

A much better prototype would be

int get_next_line(int fd, char **lineptr, size_t *sizeptr, size_t *lenptr);

where the amount of memory allocated for *lineptr would be available in *sizeptr, and the length of the actual line (not including the end-of-string \0 byte) in *lenptr. When called, if *lineptr != NULL, *sizeptr would tell how much memory is already dynamically allocated for *lineptr.

In any case, the key to implementation is to one variable to track the number of bytes stored in the new line, and another to track the number of bytes allocated to the line, and reallocate whenever necessary.

Lets assume you are required to use the first prototype shown, and that the caller is responsible for free()ing it, i.e. that the contents of *lineptr are ignored, and a new line is always allocated.

Start the function with the necessary variable declarations and checks:

int get_next_line(int fd, char **lineptr)
{
    char   *line = NULL;
    size_t  size = 0;    /* Allocated for line, remember '\0' at end */
    size_t  used = 0;    /* Number of chars stored in line */

    ssize_t n;           /* Result from read() */

    /* Just to make error handling easier, we clear *lineptr to NULL initially. */
    if (lineptr)
        *lineptr = NULL;

    /* fd must be a valid descriptor, and lineptr a valid pointer. */
    if (fd == -1 || !lineptr)
        return -1;

In the read loop, you can use n = read(fd, line + used, 1); to read the next char into the buffer, but you must obviously first ensure the buffer has sufficient room for it. So, at the beginning of the loop body, before reading the next char, you ensure the line has room for at least one char, and also for the \0 to terminate the string:

        if (used + 2 > size) {
            char *new_line;

            /* Better allocation policy! */
            size = used + 2;

            new_line = realloc(line, size);
            if (!new_line) {
                free(line);
                return -1;
            }
            line = new_line;
        }

The comment about allocation policy means that you don't really want to grow the line buffer every other character read: you'll want to grow it in larger chunks, so that reallocation is not needed so often.

What constitutes a good reallocation policy, then? It is a matter of discussion. It really depends on what the typical data is like. If the policy allocates in chunks of a megabyte or so, each line might waste a lot of memory, even if trimmed to correct size later on. (This is because many C libraries use a technique called memory mapping for large allocations -- say, 256kiB or more, but it varies from implementation to implementation -- and those have their own granularity, typically page size (which is typically a power of two, between 4kiB (4096) and 2MiB (2097152)), so on average, you have at least half a such page of wastage per line we can do nothing about.)

In practice, I recommend a policy that allocates for a typical length of line, then doubles the allocation for longer lines, up to some limit (like a megabyte or so), and allocating in multiples of that limit. For example:

            /* Suggested allocation policy, much better */
            if (used < 126)
                size = 128;
            else
            if (used < 1048574)
                size = 2 * used;
            else
                size = used + 1048576;

The exact numbers above aren't critical, as long as you keep in mind that size must grow to at least used + 2. It can grow larger, but if it does not grow to at least used + 2, we are in trouble (our program won't work right).

If read() returns 0, it means you encountered the end of file. In this case, there is nothing stored to line, and you should not increment used.

If read() returns negative, an error occurred. (Normally, read() should be able to return only -1, and in that case, errno is set to indicate the error. However, a bug in the kernel or C library is always possible -- actually, I do know of one Linux kernel bug that can cause read() to return negative, but it only occurs when trying to write larger than 2GiB chunks.)

In other words, I'd recommend something like

            n = read(fd, line + used, 1);
            if (n == 0)
                break; /* Break out of the loop; no error */
            else
            if (n < 0) {
                /* Read error of some sort;
                   errno set if n == -1 */
                free(line);
                return -1;
            }

            if (line[used] == '\n') {
                /* Keep newline character */
                used++;
                break;
            }

            /* Skip/omit embedded NUL bytes */
            if (line[used] != '\0')
                used++;

After the loop, used == 0 if nothing was read. Note that if you do not keep the newline (add the \n into the buffer), you won't be able to tell whether you read an empty line (only a \n) or if you are at the end of the line. You can avoid that by adding a flag variable, say newline_seen, that is initially zero, but you set to one. Then, only if both used and newline_seen are zero after the loop, are you at the end of input without anything to read.

After the loop, you need to append the '\0' to terminate the line properly. Also, you might wish to optimize the memory allocated to the line to the exact length:

    char *new_line;

    new_line = realloc(line, used + 1);
    if (!new_line) {
        free(line);
        return -1;
    }
    line = new_line;

    line[used] = '\0';

Finally, store the dynamically allocated line to the pointer supplied by the caller, and return. (Note that you might need to check used whether you return with 0 or 1, depending on the requirements.)

    *lineptr = line;

The above is definitely not the only way to solve this problem. For example, when you wish to omit the newline from the end of the line, you could add the terminating \0 within the loops, before the break; statements. Then, in the n == 0 case, you could check if used == 0: if it is zero, it means you are at the end of input, without anything to read, and you can free line and return (with a value indicating end of file, nothing to read).

If you compare to OP's implementation, the most important point is avoiding the secondary buffer, and using realloc() to grow the line buffer as needed.

Note that when size is a positive number (nonzero), malloc(size) and realloc(NULL, size) are equivalent. (The zero case is a bit ambiguous anyway, because some implementations return NULL, and others return a pointer that you cannot use to store other data to.)

Also, free(NULL) is safe, and does nothing. Therefore, if you initialize your pointer to NULL, use realloc() to grow it when necessary, you can always call free(pointer) to discard it, regardless of whether it is still NULL or has some memory allocated to it. In my own programs, I rarely use malloc() at all, because realloc() and free() suffice.

Finally, I am not suggesting you to copy the above code. It is important for your learning process, your learning curve, to do stuff your own way, at your own pace; otherwise, you risk dropping onto nothing. Always keep your footing secure, by fully understanding your own programs, because everything else will build upon it. Knowledge is built, not heaped. Learning by rote -- copying without understanding -- is worth nothing at this level. We stand on the shoulders of giants, and so on.

The requirement of using only malloc() is utterly silly, because realloc() is the function that is ordinarily used, and in many ways just as easy to understand.

Anyway, let's take a look at OP's implementation, and what actually happens at a get_next_line() call:

  1. ft_read_line(fd, buf, &j) is called. fd is the correct file descriptor, buf is a local array of 10,000,000 chars, and j is an int cleared to zero.

  2. ft_read_line() initializes str pointer to NULL.

  3. A while loop tries to read from the file descriptor fd into a char variable named c, as long as read() does not report an end of input. (That is, even if an error occurs.) If a newline \n is read, the code breaks out of the loop, otherwise c is appended to the buffer specified as a parameter.

  4. If str is not NULL, it is freed. This if clause is doubly useless: first, because str must be NULL here. Second, because there is no need to check if str == NULL to call free(str); free(NULL) is perfectly safe, and does nothing. (In other words, free() itself will check if its parameter is NULL, and does nothing if it is.)

  5. i is the number of characters saved into the buffer passed as a parameter. str is allocated enough memory for i+1 pointers to char. If the allocation fails, the function returns NULL.

  6. At this point, the buffer contents are treated as a string. However, no end-of-string NUL byte (\0) has been appended to the buffer, so it is not a string yet. In other words, strcpy(str, buffer) is likely to try to copy much more than i characters from buffer to str, because it looks for that NUL byte in buffer to mark the end. This is a fatal bug, and may crash the program right here.

  7. An obsolete function, bzero(), is used to clear 10,000,000 bytes in buffer to zero.

  8. The pointer str is returned to get_next_line(). Note that in get_next_line(), j will reflect the last return value from read() (that is, 1 if a character was read but was '\0', or 0 if no more input. (All other values are impossible, because those are the only two values with which the code can break out of the while loop.)

  9. The returned pointer is saved to the pointer pointed by the second parameter to get_next_line().

  10. The get_next_line() checks if j == -1. This is an useless check, because j cannot be -1 here.

  11. get_next_line() returns 0 if an end of input occurred, 1 otherwise.

As you can see, the intermediate buffer is useless. However, there are more serious issues. First is that the input data is treated as a string, without making the char array a string by appending the string-terminating NUL byte, \0. The second is that read errors are ignored, in a way that makes the while loop repeat until it tries to overwrite memory it cannot, at which point the program crashes.

How to implement the int get_next_line(int const fd, char **line) function, then, if you are only allowed to use open(), read(), malloc(), and memmove()?

The logic you need to implement is actually quite straightforward:

  1. Begin (infinite) loop:

  2. Ensure your dynamically allocated line has room for at least two chars.

  3. Try read() one character from the file descriptor to the next unused index in your dynamically allocated line.

    If read() returns

    • > 0, you have a new character. If it is a newline (\n), terminate the string by appending a NUL byte, \0. If you want your get_next_line() to remove the newline at end of each line, replace the newline read with the NUL byte. Break out of the loop.

      Note that in some cases, the file or input may contain "embedded NUL bytes", or \0 in the data itself. I personally would check for these just like newlines, except that instead of breaking out of the loop, I'd just not save them in the dynamically allocated buffer.

    • 0, no data was read, and there is no more input. If the buffer index is still zero, the end of input occurred before anything was read; so, free the dynamically allocated buffer, and return a value that indicates end-of-input (and make sure the line pointer is NULL).

      If the buffer index is not zero, it means the final line in the file did not end with a newline. Such files occasionally happen. Append '\0' to the dynamically allocated buffer, and break out of the loop.

    • < 0, an error occurred. If read() returned -1, then the exact cause is in errno; otherwise, it was a read error (same as errno == EIO). I recommend you free the dynamically allocated line (and set the line pointer to NULL) and return a value that indicates an error.

      You could, if you want, add the terminating NUL byte \0 to the current line read, set the line pointer to point to it, and return an error value; so that the caller will get a partial line even when an error occurs.

      However, in my opinion, returning a partial line when an error occurs is not sane: there is nothing safe/reliable that can be done with said line -- except possibly show it to the user, but even then, it might be garbage. Most likely, the user is only interested in knowing that an error occurred, and will throw all of the partial data away, and try something else. That's what I do (and have done).

  4. Optionally, reallocate the line to the number of chars saved in it, plus one for the (already appended) string-terminating NUL byte \0.

  5. Save the pointer to the dynamically allocated buffer (to *line), and return with a value indicating a line was successfully read.

Now, how do you ensure a dynamically allocated buffer has enough room?

Typically, we use a pointer to point to the buffer, the number of data characters in the buffer (its length), and the size allocated for the buffer (i.e. the number of characters we can store in the buffer):

char  *buffer = NULL;
size_t length = 0;
size_t allocated = 0;

When we detect that length >= allocated -- or, length + 2 > allocated, since we want to be able to add at least two chars (one data and one \0), above --, we need to reallocate a larger buffer. With realloc(), and variables initialized as above, it is as simple as

char *temp;

allocated = length + 2; /* Or, say, 2*length + 2 */
temp = realloc(buffer, allocated);
if (!temp) {
    /* Out of memory; exit program! */
}
buffer = temp;

The idea with realloc() is simple. It takes two parameters: a pointer, and the desired size.

If the pointer is NULL, then realloc() allocates memory, sufficient to store the size number of chars, and returns it.

If the pointer already points to dynamically allocated memory, realloc() grows or shrinks the amount to the desired size. If larger, all the old data is kept. If smaller, only the data up to the new size is kept. In both cases, realloc() may return a different pointer; but even so, the same data will be there.

If the pointer points anywhere else, like a local variable or array, the program will likely crash. Even if it does not crash, realloc() does not work in this case at all.

If realloc() cannot do the reallocation, it will return NULL with errno == ENOMEM. If it was trying to grow or shrink already dynamically allocated memory, that allocation and memory will still be valid. (And this is why you shouldn't use buffer = realloc(buffer, new_size): if it fails, you've lost the still-valid previous buffer. As you can see above, I use a temporary variable for the result, and only assign back to buffer if it is non-NULL.)

We can write our own analog of realloc() using malloc() and memmove(), though. We just need to know both the old and new sizes:

void *our_realloc(void *old_data, const size_t old_size, const size_t new_size)
{
    void *new_data;

    /* Reallocation to zero size is special. We always free and return NULL. */
    if (new_size < 1) {
        free(old_data);
        return NULL;
    }

    /* Allocate memory for the new buffer. */
    new_data = malloc(new_size);
    if (!new_data)
        return NULL; /* Failed! */

    /* Copy old data, if any. */
    if (old_size > 0) {
        if (old_size < new_size)
            memmove(new_data, old_data, old_size);
        else
            memmove(new_data, old_data, new_size);
    }

    free(old_data);

    return new_data;
}

The real realloc() is superior, because it does not need to know the old size (the C library remembers it internally!), and it can usually grow/shrink the allocation in place, without temporarily needing additional memory like we do above.

I fear the lecturer or course designer is either a genius or a functional idiot, and instead thought that you should write the get_next_line() along the following lines:

  1. Allocate a dynamic buffer you think is large enough.

  2. In a loop:

  3. read() one character. If read() returns:

    • > 0, you read an additional character.

      If that character is \n, append it to the buffer (if you wish to keep the newlines), and make the buffer contents into a string by appending the string-terminating NUL byte \0, too. Save the buffer pointer and return (value indicating a new line was read).

      If that character is anything other than \0, append it to the dynamic buffer. (Since \0 terminates a string, we skip them if we read them from the file.)

    • == 0, there is nothing more to read. If the dynamically allocated buffer does not have any data in it yet, we got end-of-input before any data, and can discard the buffer, and return "nothing; end of input".

    • < 0, an error occurred. Discard the dynamically allocated buffer, and return read error.

Note that in the above case, you can use an infinite loop. For example, while (1) { ... } or for (;;) { ... }. You can break out of them, but here, you can also just return from the entire function at the above cases.

If they use this exercise to show how malloc() alone leads to arbitrary magic constants and buffer sizes, and how important realloc() is, and how relatively easily you can fix this to work for any-length lines (with realloc()`, they are a genius. Because real-world code has to deal with such stuff, and it might actually be a working approach to teaching dynamic memory management.

If they think this kind of code is acceptable in any other case, they are a functional idiot. The amount of time and resources lost due to badly-written software (for example, the kind that has secret limits that cause it to crash without any explanation if you happen to exceed those) is astronomical, and adding to that body is like teaching students how to mix papercuts hand household chemicals for maximum pain, and use it to gain sympathy. Functional, but idiotic.

Upvotes: 2

Related Questions