1

I've the following code where basically I implemented my own read-line function for exercising me in memory allocation, etc, in C. Before I asked a question, but no one actually helped in trying to correct my code eventually except for suggesting to use valgrind. Since I'd never used it before, it's quite hard for me to understand everything.

My code is the following:

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

/**
Gets and a variable-size line from the standard input.
*/
char* readline(){

    size_t n = 10;
    char* final = calloc(n, sizeof(char));
    final[0] = '\0';
    char* tmp; // used for allocating memory temporarily

    // constant buffer size used to store the read characters
    // before storing them in the final buffer
    char buf[10]; 

    while(fgets(buf, 10, stdin) != NULL) {

        if(buf[strlen(buf) - 1] == '\n') {

            if(strlen(buf) > 1) {

                if((n - strlen(final)) < (strlen(buf) + 1)) {
                    // -1 because buf contains also \n at the end
                    n = strlen(final) + strlen(buf);
                    tmp = calloc(n, sizeof(char));

                    for(int i=0; i <= strlen(final); ++i)
                        tmp[i] = final[i];

                    free(final);
                } else {
                    tmp = final;
                }

                int i, j;
                for(i = strlen(final), j = 0; j <= (strlen(buf) - 2); ++i, ++j)
                    tmp[i] = buf[j];

                tmp[i] = '\0';

                final = tmp;
                tmp = NULL;
            }

            break;

        } else { // no newline inserted at the end

            if((n - strlen(final)) < (strlen(buf) + 1)) {
                n *= 2;
                tmp = calloc(n, sizeof(char));

                for(int i = 0; i <= strlen(final); ++i)
                    tmp[i] = final[i];

                free(final);

            } else {
                tmp = final;
            }       

            // Starts inserting from the '\0' char
            // Insert also the '\0' at the end
            for(int i = strlen(tmp), j = 0; j <= 9; ++i, ++j)
                tmp[i] = buf[j];

            final = tmp;
            tmp = NULL;
        }
    }

    return final;
}



int main(int argc, char *argv[]){

    if(argc < 2){
        fprintf(stderr, "usage: at least one string as command-line argument.\n");
        exit(1);
    } else {
        char* line = readline();
        printf("line = %s\n", line);
        printf("size = %lu\n", strlen(line));
        free(line);
    }

    return 0;
}

When I run valgrind with the command:

valgrind ./findword hello

I get the following ouput

==14084== Memcheck, a memory error detector
==14084== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==14084== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==14084== Command: ./findword hello
==14084== 
hello world, how are you?
==14084== Invalid read of size 1
==14084==    at 0x10000A669: strlen (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000C19: readline (findword.c:46)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084==  Address 0x100a78740 is 0 bytes inside a block of size 20 free'd
==14084==    at 0x10000927F: free (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000C03: readline (findword.c:40)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084==  Block was alloc'd at
==14084==    at 0x100009541: calloc (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000D0F: readline (findword.c:61)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084== 
==14084== Invalid read of size 1
==14084==    at 0x10000A672: strlen (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000C19: readline (findword.c:46)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084==  Address 0x100a78742 is 2 bytes inside a block of size 20 free'd
==14084==    at 0x10000927F: free (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000C03: readline (findword.c:40)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084==  Block was alloc'd at
==14084==    at 0x100009541: calloc (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==14084==    by 0x100000D0F: readline (findword.c:61)
==14084==    by 0x100000E6C: main (findword.c:93)
==14084== 
line = hello world, how are you?
size = 25
==14084== 
==14084== HEAP SUMMARY:
==14084==     in use at exit: 30,666 bytes in 189 blocks
==14084==   total heap usage: 276 allocs, 87 frees, 36,962 bytes allocated
==14084== 
==14084== LEAK SUMMARY:
==14084==    definitely lost: 0 bytes in 0 blocks
==14084==    indirectly lost: 0 bytes in 0 blocks
==14084==      possibly lost: 2,064 bytes in 1 blocks
==14084==    still reachable: 4,096 bytes in 1 blocks
==14084==         suppressed: 24,506 bytes in 187 blocks
==14084== Rerun with --leak-check=full to see details of leaked memory
==14084== 
==14084== For counts of detected and suppressed errors, rerun with: -v
==14084== ERROR SUMMARY: 19 errors from 2 contexts (suppressed: 0 from 0)

So, apparently, I've a lot of errors, but I'm not managing to find them. For example, valgrind is claiming Invalid read of size 1, but I see nowhere where I'm reading a wrong location in memory which produces undefined behaviour.

Edit

I've recompiled my code with 

 gcc -g -o findword findword.c

And I've replaced the new valgrind output above.

nbro
  • 15,395
  • 32
  • 113
  • 196
  • 1
    You should probably compile with debug information (`-g`). – EOF Mar 14 '16 at 22:42
  • It would be nice if you compiled with debug symbols, so at least we'd have line numbers in the report. – Oliver Charlesworth Mar 14 '16 at 22:42
  • @EOF Ok, I will do it and then edit the question. – nbro Mar 14 '16 at 22:44
  • The line numbers in the valgrind output don't seem to match the lines in the posted code. Could you post the code exactly as you compiled it, or at least annotate the relevant lines with a comment? – EOF Mar 14 '16 at 22:56

1 Answers1

2

Well, for one: you calloc a new buffer:

tmp = calloc(n, sizeof(char));

and copy the contents:

for(int i=0; i <= strlen(final); ++i)
   tmp[i] = final[i];

and free the final:

free(final);

But you do not assign a new pointer final, thus now final points to freed memory, yet later you do strlen() on that.

Also do not call strlen() all the time, it is very slow. Especially not in the loop conditions. Use strcpy or strncpy to copy strings to new arrays instead of looping. Use realloc for resizing the memory area instead of callocing. See my example that you didn't want to see.

Community
  • 1
  • 1
Antti Haapala -- Слава Україні
  • 129,958
  • 22
  • 279
  • 321
  • It's correct, I'm using `strlen` on a just freed `final`... I meant `strlen(tmp)`...but apart from that, that's not a problem, right? – nbro Mar 14 '16 at 22:50
  • I'm assigning a `tmp` then to `final` at the end of the if or else statement...my question was, apart from accessing `strlen(final)`, there are not other problems right? – nbro Mar 14 '16 at 22:51
  • I am not sure if it is the **only** problem, very probably that is not the only problem. You should fix it so that the valgrind output starts making sense. – Antti Haapala -- Слава Україні Mar 14 '16 at 22:51
  • Yes, that was apparently the only error... Now valgrind doesn't complain `...ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)...` :) Thanks for the suggestions anyway :) – nbro Mar 14 '16 at 22:54
  • Actually, you're pointing out the wrong loop where I use `strlen` on the just freed memory pointed by `final`... in the loop you're mentioning, `final` is not freed... – nbro Mar 14 '16 at 22:56
  • Actually the error was just in the loop of the first `if` (i.e., `for(i = strlen(final), j = 0; j <= (strlen(buf) - 2); ++i, ++j)`) , in the loop of the `else` I'm using `tmp` (i.e., `for(int i = strlen(tmp), j = 0; j <= 9; ++i, ++j)`), as I intended, but not did... – nbro Mar 14 '16 at 23:00
  • 1
    I'm going to follow your suggestions and try to improve it ;) Thanks! – nbro Mar 14 '16 at 23:05
  • `Also do not call strlen() all the time, it is very slow. Especially not in the loop conditions.` GLIBC (at least) uses `__const__` function attribute, so no, this is basically equivalent to 1 call. –  Mar 14 '16 at 23:17
  • @user9000 perhaps, wouldn't rely on compiling on such platforms always. – Antti Haapala -- Слава Україні Mar 14 '16 at 23:27
  • @user9000 and I am not too sure that the compiler understands that `tmp` and `final` there are distinct objects. – Antti Haapala -- Слава Україні Mar 14 '16 at 23:33
  • @user9000 compiled the code, the resulting object code has *14* calls to strlen, and clearly in loops – Antti Haapala -- Слава Україні Mar 14 '16 at 23:37
  • @AnttiHaapala I don't know if I did a mistake or not yesterday when checking, but valgrind today is still giving me one error...`2,064 bytes in 1 blocks are possibly lost in loss record 57 of 64`... I will edit my question above with the error.. – nbro Mar 15 '16 at 09:33
  • @nbro please ask another – Antti Haapala -- Слава Україні Mar 15 '16 at 09:34