7

I've wrote this small function to read all the data from stdin.

I need to know if this function is POSIX compatible (by this, I mean it will work under Unix and Unix-like systems) at least it works on Windows...

char* getLine()
{
    int i = 0, c;
    char* ptrBuff = NULL;

    while ((c = getchar()) != '\n' && c != EOF)
    {
        if ((ptrBuff = (char*)realloc(ptrBuff, sizeof (char)+i)) != NULL)
            ptrBuff[i++] = c;
        else
        {
            free(ptrBuff);
            return NULL;
        }
    }

    if (ptrBuff != NULL)
        ptrBuff[i] = '\0';

    return ptrBuff;
}

The function reads all the data from stdin until get '\n' or EOF and returns a pointer to the new location with all the chars. I don't know if this is the most optimal or safer way to do that, and neither know if this works under Unix and Unix-like systems... so, I need a little bit of help here. How can I improve that function? or is there a better way to get all the data from stdin without leaving garbage on the buffer? I know that fgets() is an option but, it may leave garbage if the user input is bigger than expected... plus, I want to get all the chars that the user has written.

EDIT:

New version of getLine():

char* readLine()
{
    int i = 0, c;
    size_t p4kB = 4096;
    void *nPtr = NULL;
    char *ptrBuff = (char*)malloc(p4kB);

    while ((c = getchar()) != '\n' && c != EOF)
    {
        if (i == p4kB)
        {
            p4kB += 4096;
            if ((nPtr = realloc(ptrBuff, p4kB)) != NULL)
                ptrBuff = (char*)nPtr;
            else
            {
                free(ptrBuff);
                return NULL;
            }
        }
        ptrBuff[i++] = c;
    }

    if (ptrBuff != NULL)
    {
        ptrBuff[i] = '\0';
        ptrBuff = realloc(ptrBuff, strlen(ptrBuff) + 1);
    }

    return ptrBuff;
}

LAST EDIT:

This is the final version of the char* readLine() function. Now I can't see more bugs neither best ways to improve it, if somebody knows a better way, just tell me, please.

char* readLine()
{
    int c;
    size_t p4kB = 4096, i = 0;
    void *newPtr = NULL;
    char *ptrString = malloc(p4kB * sizeof (char));

    while (ptrString != NULL && (c = getchar()) != '\n' && c != EOF)
    {
        if (i == p4kB * sizeof (char))
        {
            p4kB += 4096;
            if ((newPtr = realloc(ptrString, p4kB * sizeof (char))) != NULL)
                ptrString = (char*) newPtr;
            else
            {
                free(ptrString);
                return NULL;
            }
        }
        ptrString[i++] = c;
    }

    if (ptrString != NULL)
    {
        ptrString[i] = '\0';
        ptrString = realloc(ptrString, strlen(ptrString) + 1);
    } 
    else return NULL;

    return ptrString;
}
  • 1
    See this blog post: http://powerfield-software.com/?p=65 - it's also been used as an answer on a couple of question here on SO. – paxdiablo Nov 24 '14 at 04:46
  • Yes, your function is POSIX, I tested it with gcc in Linux, and seems to work as expected. – Jose Rey Nov 24 '14 at 04:50
  • 1
    Allocating one byte at a time like that can lead to quadratic performance — which is not good. You should allocate blocks of data at a time, perhaps doubling the space allocated each time you need more space. You should also note that `ptrBuff = realloc(ptrBuff, new_size);` leaks memory when allocation fails, because you write a NULL over the only pointer to the previous memory allocation. The `free(ptrBuff);` is safe because `free(NULL)` is a no-op. It doesn't free the memory because `ptfBuff` no longer points at it. Use `void *new_space = realloc(ptrBuff, new_size);` and check for nullness. – Jonathan Leffler Nov 24 '14 at 04:58
  • 1
    The simple POSIX-compliant solution, incidentally, is to use [`getline()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html). That allocates memory as necessary too. – Jonathan Leffler Nov 24 '14 at 05:00
  • Your code works works absolutely fine in the unix environment. But, Just as Jonathan Leffler pointed out your code is not optimistic and might cause memory leak. You can allocate some amount of memory and when memory is required you can add to it. If you are sure of an upper bound then the best and safest function to use is fgets. – Kranthi Kumar Nov 24 '14 at 05:16
  • Hi @paxdiablo I've already see that url, but that's the case when the programmer set the limit of chars to be read, I need to read the whole line and work with it. So, for now... I think my function do the trick as well, anyways thank you for replying. –  Nov 24 '14 at 05:57
  • Thanks @JoseRey. I'm glad about that. –  Nov 24 '14 at 06:06
  • @JonathanLeffler Nice, I've tried to fix that. Now it uses 4kb of memory, due Linux and Windows uses 4kb pages to divide the virtual memory ***AFAIK**. I've also deleted the `free(ptrBuff)` instruction, since it wasn't doing anything useful. And I've declared a new pointer to store the new memory chunks and check for nullness before any pointer changes... so the program will return the last **valid** pointer with **valid** data. Have I missed something? PS: I'm still learning... I'm reading the K&R book. –  Nov 24 '14 at 07:13
  • You now have: `if ((newSpace = realloc(ptrBuff, p4kb)) == NULL) ptrBuff = (char*)newSpace; else`. The intention was that you should have: `if ((newSpace = realloc(ptrBuff, p4kb)) == NULL) { free(ptrBuff); return 0; } else`. You might also want to avoid using the name `getline()` since it is a POSIX standard function. I went through a painful process when I found out that this name had been standardized — but it is harder to fight the standard than to comply. – Jonathan Leffler Nov 24 '14 at 07:15
  • @KranthiKumar No, I don't want a limited string, I want the entire line from stdin... with no garbage after it. I've modified the code, I hope it works better now... –  Nov 24 '14 at 07:20
  • 1
    @JonathanLeffler Sorry, my mistake, the line actually was: `if ((newSpace = realloc(ptrBuff, p4kb)) != NULL)` but I was testing while debugging to see what could happen... now I doubt if I need to use the `free()` function or not, because when `realloc()` fails, `newSpace` will contain a null pointer, and `ptrBuff` will contain the last valid pointer with valid data... –  Nov 24 '14 at 07:23
  • OK; that makes more sense…it gets tricky when you need to return 'OK', 'Truncated' and 'EOF' statuses, where your 'return on reallocation failure' is the 'Truncated' status. You have to decide what to do. Releasing the allocated memory and reporting failure by returning NULL (or 0) is the other way to do it. But that decision is yours to make. – Jonathan Leffler Nov 24 '14 at 07:25
  • @JonathanLeffler If I return a NULL pointer, indicating there was an error while realocating memory, then there'll be useless the `newSpace` pointer... am I wrong? Because I only use the `newSpace` pointer to check if I can allocate more memory, and thus... realocate the last `ptrBuff`. But if I can't allocate more memory, then `newSpace` will be NULL and the function returns the last readed chars. I think that's the propoose of newSpace, isn't? –  Nov 24 '14 at 07:37
  • The purpose of `newspace` is to allow you to free `ptrBuff` if the reallocation fails — or to return `ptrBuff` if the reallocation fails. Which you do is a design decision that must be documented to your users — either choice is valid and neither is decisively better than the other in all circumstances. – Jonathan Leffler Nov 24 '14 at 07:39
  • @JonathanLeffler Ok!, now I've decided to return a NULL value if reallocation fails, and so... `free(ptrBuff)`. This code is not for any users, I'm just a neophite who wants to read correctly a whole line from stdin... thank you for all! Now I get a correct way to read the stdin. –  Nov 24 '14 at 07:47
  • Finally! I've modified the code as well as I could. At the end, the snippet releases all the memory that was allocated for the buffer, but that wasn't used. Hope it works for somebody. –  Nov 24 '14 at 08:20

2 Answers2

4

POSIX-compatible: yes!

You're calling only getchar(), malloc(), realloc() and free(), all of which are standard C functions and therefore also available under POSIX. As far as I can tell, you've done all the necessary return code checks too. Given that, the code will be good in any environment that supports malloc() and stdin.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
user313114
  • 311
  • 1
  • 3
0

Only thing I would change is the last call to strlen, which is not necessary since the length is already stored in i.

Benjamin Schmid Ties
  • 11
  • 1