Fix: 'record' not freed upon failure

[andgera]

I checked the code with cppcheck and got an error:

Checking cmusfm/src/cache.c ...
cmusfm/src/cache.c:84:2: error: Common realloc mistake: 'record' nulled but not freed upon failure [memleakOnRealloc]
record = (struct cmusfm_cache_record *)realloc(record, get_cache_record_size(record));

[arkq]

It's not waisted time, you've learned something :) If you like, you can fix this error handling. But like I've said, no exit(). Using exit is better than undefined behavior, but in that case there will be no UB, simply cmusfm will segfault. And since it's possible to handle mem error correctly (cache can be submitted later), it's better to do that.

[andgera]

yes, initially my code looked like this:

        if (record == NULL) {
                perror("ERROR: error enlarge allocated memory");
        free(record);
        return NULL;

but, since cppcheck still gave an error,

Checking cache.c ...
cache.c:84:2: error: Common realloc mistake: 'record' nulled but not freed upon failure [memleakOnRealloc]
 record = (struct cmusfm_cache_record *)realloc(record, get_cache_record_size(record));

I decided that then exit(EXIT_FAILURE); and cppcheck showed that there are no errors :)

[arkq]

yes, initially my code looked like this:

The problem here is that the variable which is passed to realloc is overwritten by the same call to realloc, so in case of realloc failure one will loose pointer to previous memory chunk - it will be impossible to free it, hence the warning report.

The code should look like this:

struct cmusfm_cache_record *tmp = record;
if ((record = realloc(tmp, get_cache_record_size(record))) == NULL) {
    free(tmp);
    return NULL;
}

[andgera]

if you are guided by the output of cppcheck, then need to remove the release of the free(tmp); memory area

$ cppcheck --language=c ./cache.c                                                                                                                 
Checking cache.c ...
cache.c:87:3: error: Memory pointed to by 'tmp' is freed twice. [doubleFree]
  free(tmp);
  ^
cache.c:86:16: note: Memory pointed to by 'tmp' is freed twice.
 if ((record = realloc(tmp, get_cache_record_size(record))) == NULL) {
               ^
cache.c:87:3: note: Memory pointed to by 'tmp' is freed twice.
  free(tmp);
  ^

not much off topic: if we look more broadly, then I'm probably confused and still don't understand why you need to check realloc() at all, here in man 3 realloc it says: "The returned pointer may be the same as ptr if the allocation was not moved (e.g., there was room to expand the allocation in-place), or different from ptr if the allocation was moved to a new address."

ISO/IEC 9899:201x goes on to say: "The realloc function returns a pointer to the new object (which may have the same value as a pointer to the old object), or a null pointer if the new object could not be allocated ."

From all of the above, the most correct thing is, as you said earlier, to check malloc(), and if malloc allocated memory, then in case of failure, realloc() will return the old value of malloc.

While writing, I began to understand why without the malloc() check, the realloc() check does not make sense :) It seemed to be clear and so, but then I got into the essence of what was happening.

I'm sorry to waste your time and attention, it might be quite amusing for you to watch.

[arkq]

You have to check realloc and malloc because both these functions MAY return null. It may happen or may not. There is a distinction in manuals and RFCs between may and shall. If realloc fails it returns null, but the old memory chunk is not changed, you can still use it. However, in this case we can not use it, because it's too short for our data - we need that reallocation. So, in order to report error we will return null, but the chunk allocated with malloc has to be freed, hence free befor return.

[andgera]

Yes! now everything is clear. We don't need the old malloc value. Thank you for putting everything in my head in its place.

[andgera]

then it should look like this: cppcheck found no errors

diff --git a/src/cache.c b/src/cache.c
index 07791bd..52c5825 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -63,6 +63,10 @@ static struct cmusfm_cache_record *get_cache_record(const scrobbler_trackinfo_t

    /* allocate memory for the initial record data (header) */
    record = (struct cmusfm_cache_record *)calloc(1, sizeof(*record));
+   if (record = NULL) {
+       free(record);
+       return NULL;
+   }

    record->signature = CMUSFM_CACHE_SIGNATURE;
    record->timestamp = sb_tinf->timestamp;
@@ -81,7 +85,11 @@ static struct cmusfm_cache_record *get_cache_record(const scrobbler_trackinfo_t
        record->len_mb_track_id = strlen(sb_tinf->mb_track_id) + 1;

    /* enlarge allocated memory for string data payload */
-   record = (struct cmusfm_cache_record *)realloc(record, get_cache_record_size(record));
+   struct cmusfm_cache_record *tmp = record;
+   if ((record = realloc(tmp, get_cache_record_size(record))) == NULL) {
+       free(record);
+       return NULL;
+}
    ptr = (char *)&record[1];

    if (record->len_artist) {

[arkq]

cppcheck found no errors

..... but there are errors. I think that clang will report a typo you've made: record = NULL. Also, there is no point calling free(variable) if you know that the variable is NULL... you shall free pointer which is not null.

[andgera]

Yes, you are absolutely right clang did report a mistake I made :smile: and even suggested maybe ==

$ clang-9 src/cache.c                                                                                                                                 
src/cache.c:66:13: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]                                                                          
        if (record = NULL) {                                                                                                                                                              
            ~~~~~~~^~~~~~                                                                                                                                                                 
src/cache.c:66:13: note: place parentheses around the assignment to silence this warning                                                                                                  
        if (record = NULL) {                                                                                                                                                              
                   ^                                                                                                                                                                      
            (            )                                                                                                                                                                
src/cache.c:66:13: note: use '==' to turn this assignment into an equality comparison                                                                                                     
        if (record = NULL) {                                                                                                                                                              
                   ^                                                                                                                                                                      
                   ==                

if write like this, then the free(tmp) pointer is freed, but cppcheck gives errors (but can you trust it)

        struct cmusfm_cache_record *tmp = record;
        if ((record = realloc(tmp, get_cache_record_size(record))) == NULL) {
        free(tmp);
        return NULL;
        }

then just return NULL

        record = (struct cmusfm_cache_record *)calloc(1, sizeof(*record));
        if (record = NULL) {
        return NULL;
        }

I understand that I am more trouble than good, and I am grateful to you!

[arkq]

See this: https://sourceforge.net/p/cppcheck/discussion/general/thread/1bff79c14a/?limit=25

[andgera]

Ok! Thank you! In version Cppcheck 2.3 this problem was not solved.

[andgera]

Sorry for the delay in making changes. There were urgent matters.

[arkq]

OK, no problem. But now since the get_cache_record() is fixed, you will have to add check for NULL in place where get_cache_record() is actually used.

[andgera]

Ok, I will offer this code.

diff --git a/src/cache.c b/src/cache.c
index 175975d..c1d0e1b 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -134,7 +134,11 @@ void cmusfm_cache_update(const scrobbler_trackinfo_t *sb_tinf) {
    if ((f = fopen(cmusfm_cache_file, "a")) == NULL)
        return;

-   record = get_cache_record(sb_tinf);
+   if ((record = get_cache_record(sb_tinf)) == NULL) {
+       fclose(f);
+       return;
+   }
+
    record_size = get_cache_record_size(record);

    /* convert host endianness to the "universal" one */

[arkq]

Yes, something like that should be OK.

In general there should be some kind of a feedback to the user that something was not right and entry will not be stored in the cache... but the output from the cmusfm will not be visible to the user anyway... Also, there is no error message when fopen() fails... What can I say... old code :D

[andgera]

Сan write to the standard error stream, though I don’t know how efficient it is, but it's better than nothing at all? Can add to messages for debugging?

diff --git a/src/cache.c b/src/cache.c
index 175975d..09e72b0 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -131,10 +131,17 @@ void cmusfm_cache_update(const scrobbler_trackinfo_t *sb_tinf) {
            sb_tinf->artist, sb_tinf->album, sb_tinf->album_artist,
            sb_tinf->track_number, sb_tinf->track, sb_tinf->duration);

-   if ((f = fopen(cmusfm_cache_file, "a")) == NULL)
+   if ((f = fopen(cmusfm_cache_file, "a")) == NULL) {
+       fprintf(stderr, "ERROR: can't open file for writing\n");
        return;
+   }
+
+   if ((record = get_cache_record(sb_tinf)) == NULL) {
+       fclose(f);
+       fprintf(stderr, "ERROR: Invalid cache data\n");
+       return;
+   }

-   record = get_cache_record(sb_tinf);
    record_size = get_cache_record_size(record);

    /* convert host endianness to the "universal" one */
@@ -166,8 +173,10 @@ void cmusfm_cache_submit(scrobbler_session_t *sbs) {

    debug("Cache submit");

-   if ((f = fopen(cmusfm_cache_file, "r")) == NULL)
+   if ((f = fopen(cmusfm_cache_file, "r")) == NULL) {
+       fprintf(stderr, "ERROR: can't open file for reading\n");
        return;
+   }

    /* read file until EOF */
    while (!feof(f)) {

[arkq]

Сan write to the standard error stream, though [...]

No, I think that it's not required. Whole cmusfm code should be reviewed for potential useful logging. But this task is not related to this PR. So, simple return is OK.