Global variable and realloc and memory leak

Question

I would like to know why valgrind says:

    ==9952== 30 bytes in 6 blocks are definitely lost in loss record 1 of 1
    ==9952==    at 0x4C2BF0E: realloc (vg_replace_malloc.c:662)
    ==9952==    by 0x40131F: setCharsPositions (paramsExec.c:99)
    ==9952==    by 0x400CF3: main (main.c:87)

I can't figure out what's the problem with my realloc() (you don't have to say that reallocating memory by one every time is inefficient...); the variable char **passwordSet2 is global — maybe that is the problem... If I'm doing something wrong, please let me know! I'm going crazy!

void setCharsPositions(char *charsPos){

    int i, k;
    char *posStr = NULL;

    for(i = 0; i < strlen(charsPos); i++){

        posStr = malloc(sizeof(char));

        if(charsPos[i] == '['){

            for(k = 0, i++; charsPos[i] != ','; i++, k++){
                posStr[k] = charsPos[i];
                posStr = realloc(posStr, (k+2)*sizeof(char));
            }
            posStr[k] = '\0';

            passwordSet2[atoi(posStr)-1] = malloc(sizeof(char));
            for(k = 0, i++; charsPos[i] != ']'; i++, k++){
                passwordSet2[atoi(posStr)-1][k] = charsPos[i];
                passwordSet2[atoi(posStr)-1] = realloc(passwordSet2[atoi(posStr)-1], (k+2)*sizeof(char));
            }
            passwordSet2[atoi(posStr)-1][k] = '\0';
        }
        free(posStr);
    }
}

`sizeof (char)` is always 1; You're unnecessarily multiplying by 1. Additionally, a minimal, compilable testcase would be nice. Consider writing a `main` entry point to make this compilable. — autistic, Jul 09 '13 at 04:33
instead of using `atoi` in many places, you can store it in int variable like `posInt` — VoidPointer, Jul 09 '13 at 04:34
@undefinedbehaviour won't the compiler optimize that _multiply by 1_? Just for my knowledge.. — VoidPointer, Jul 09 '13 at 04:38
@VoidPointer Optimisation for runtime efficiency is far less important than optimisation for human readability, in this case. — autistic, Jul 09 '13 at 04:41
@VoidPointer The multiply doesn't cost anything, it's just pointless clutter. — Jim Balter, Jul 09 '13 at 04:41

Jim Balter · Accepted Answer · 2013-07-09T04:56:20.880

-1

passwordSet2[atoi(posStr)-1] = malloc(sizeof(char));

If passwordSet2[atoi(posStr)-1] was previously assigned, this leaks memory. That is, you have a leak if the same value of posStr occurs more than once. The lost memory would be from a previous realloc, so that's what valgrind will report. (It would help if you would point out which is line 662.) If the same value of posStr is occurring more than once but isn't supposed to, then you need to check for that. If it's allowed to occur more than once, then you should add a call to free:

free(passwordSet2[atoi(posStr)-1]);
passwordSet2[atoi(posStr)-1] = malloc(sizeof(char));

The first occurrence of free, when passwordSet2[atoi(posStr)-1] is NULL, is a no-op.

Also, you're not checking for NULL returns from malloc and realloc, though those would cause your program to crash, not just leak.

Finally, for clarity and possibly efficiency, I would urge you to put atoi(posStr)-1 into a variable instead of repeating it.

edited Jul 09 '13 at 04:56

answered Jul 09 '13 at 04:35

Jim Balter

16,163
3
43
66

FWIW, to better complete this answer, supposing realloc were to return `NULL`, there'd also be a leak here: `posStr = realloc(posStr, (k+2)*sizeof(char));` – autistic Jul 09 '13 at 04:43
It wasn't previously assigned, well, I initialized the passwordSet2 variable like this: for(i = 0; i < max; i++) passwordSet2[i] = NULL; where max is another global variable... – Paolo Carrara Jul 09 '13 at 04:44
@undefinedbehaviour There would also be UB, most likely an access violation, in the immediately following store, so I don't think that adds much because the question isn't about checking for NULL returns. – Jim Balter Jul 09 '13 at 04:45
@pcarrara Like what? Anyway, I'm not talking about initialization ... see my additional words. – Jim Balter Jul 09 '13 at 04:46
@JimBalter Yes, that's true. I guess the incorrect use of `realloc` could be addressed most quickly by recommending a visit to a question like ["understanding realloc"](http://stackoverflow.com/questions/3684288/understanding-realloc). – autistic Jul 09 '13 at 04:47
Ok, I'm going to read the question about "understanding realloc", maybe, I could better understand my problem... Just to know, UB is undefinedbehavior? – Paolo Carrara Jul 09 '13 at 04:53
@pcarrara indeed, yes UB => Undefined Behavior – VoidPointer Jul 09 '13 at 04:55
Well, I already had checked if my realloc isn't working, but I always get the same error, ie thats not the problem... I don't think that I'm accessing more than once the same position of my matrix... – Paolo Carrara Jul 09 '13 at 05:00
@pcarrara Your realloc is working fine, you're just losing its return value at some point. "I don't think that I'm accessing more than once the same position of my matrix..." -- I would triple check that because it would explain the error and nothing else would. Put in a test to see if passwordSet2[atoi(posStr)-1] is NULL before setting it to malloc(...); if it isn't, that's your leak. – Jim Balter Jul 09 '13 at 05:04
Thanks everyone to so fast try to solve my problem! You are really awesome! I'll try to solve the problem by following your point of view, if I can't get to the answer you will hear me again ;) (thanks and sorry for my bad english, I'm from Brazil) – Paolo Carrara Jul 09 '13 at 05:11

Global variable and realloc and memory leak

1 Answers1