C - Can't get rid of memory leaks

Question

I did the program which reads words from one file, sorts them and inserts into list, then I print the list into new file. Words have to be sorted alphabetically. I used strcmp() function to compare them and I created function MakeLowerCase(). It "converts" string to lowercase string. I know that program make a lot of operations but I have no idea how could I optimize it. Anyway, I have to get rid of memory leaks, but I don't know where it leaks.

I've got such report from Dr Memory (only part of it):

error #5: LEAK 2 direct bytes 0x01373d60-0x01373d62 + 0 indirect bytes
# 0 replace_malloc                     [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 msvcrt.dll!_strdup   
# 2 .text      
# 3 __mingw_glob
# 4 _setargv   
# 5 .text      
# 6 ntdll.dll!__RtlUserThreadStart

Error #6: LEAK 11 direct bytes 0x01373db0-0x01373dbb + 0 indirect bytes
# 0 replace_malloc               [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 MakeItTxt  
# 2 main       

Error #7: LEAK 6 direct bytes 0x01374f10-0x01374f16 + 0 indirect bytes
# 0 replace_malloc                    [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 MakeLowerCase
# 2 sortedInsert
# 3 InsertWordsToStruct
# 4 main       

Error #8: LEAK 6 direct bytes 0x01374f38-0x01374f3e + 0 indirect bytes
# 0 replace_malloc                    [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 MakeLowerCase
# 2 sortedInsert
# 3 InsertWordsToStruct
# 4 main       

Error #9: LEAK 4 direct bytes 0x013750a0-0x013750a4 + 0 indirect bytes
# 0 replace_malloc                    [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 MakeLowerCase
# 2 sortedInsert
# 3 InsertWordsToStruct
# 4 main       

Error #10: LEAK 6 direct bytes 0x013750c8-0x013750ce + 0 indirect bytes
# 0 replace_malloc                    [d:\drmemory_package\common\alloc_replace.c:2577]
# 1 MakeLowerCase
# 2 sortedInsert
# 3 InsertWordsToStruct
# 4 main       

Reached maximum leak report limit (-report_leak_max). No further leaks will be reported.

===========================================================================
FINAL SUMMARY:

DUPLICATE ERROR COUNTS:
    Error #   1:      8
    Error #   2:      8
    Error #   4:      3
    Error #   7:    137
    Error #   8:    137
    Error #   9:   4855
    Error #  10:   4854

SUPPRESSIONS USED:

ERRORS FOUND:
      0 unique,     0 total unaddressable access(es)
      3 unique,    17 total uninitialized access(es)
      0 unique,     0 total invalid heap argument(s)
      0 unique,     0 total GDI usage error(s)
      0 unique,     0 total handle leak(s)
      0 unique,     0 total warning(s)
      7 unique,  9988 total,  68700 byte(s) of leak(s)
      0 unique,     0 total,      0 byte(s) of possible leak(s)
ERRORS IGNORED:
     10 unique,    12 total,    421 byte(s) of still-reachable allocation(s)

I can share with you part of my code (the functions from report):

char* MakeLowerCase(char* word)
{
  char* lower = (char *)malloc(sizeof(char)*strlen(word)+1);
  strcpy(lower, word);
  int i = 0;

  for(i = 0; i < strlen(lower); i++){
    lower[i] = tolower(lower[i]);
  }
  return lower;
}

Word* newItem(char* word)
{
    /* allocate node */
    Word* new_item = (Word *)malloc(sizeof(Word));
    int word_length = strlen(word);
    new_item->word = (char *)malloc((word_length+1)*sizeof(char));

    strcpy(new_item->word, word);
    new_item->pNext = NULL;

    return new_item;
}

void sortedInsert(Word** pH, Word* new_node)
{
    Word* current;
    /* Special case for the head end */
    if (*pH == NULL || strcmp(MakeLowerCase((*pH)->word), MakeLowerCase(new_node->word)) == 1)
    {
        new_node->pNext = *pH;
        *pH = new_node;
    }
    else
    {
        /* Locate the node before the point of insertion */
        current = *pH;
        while (current->pNext!=NULL &&
               strcmp(MakeLowerCase(current->pNext->word), MakeLowerCase(new_node->word)) == -1)
        {
            current = current->pNext;
        }
        new_node->pNext = current->pNext;
        current->pNext = new_node;
    }
}

void InsertWordsToStruct(Word** pH, FILE* filename){
  char single_line[16384]; /* longest posible line in my code */

  int number_of_words = 0;
  int counter = 0;

  while(fgets(single_line, 16384, filename))
  {
    char* single_word = strtok(single_line, " \t\n\0"); /* cut one word from line */
    while(single_word != NULL)
    {
      if(IsLegitWord(single_word) == true) /* function return true if word is really word */
      {
        Word* new_node = newItem(single_word);
        sortedInsert(pH, new_node);
      }
      single_word = strtok(NULL, " \t\n\0");
    }
  }
}

I really cannot find the leaks. I am new in C, sorry. Of course, at the end of the file I use in main:

RemoveWordList(&listname);

Function:

void RemoveWordList(Word** pH){
  Word* current = *pH;
  while (current != NULL){
      Word* next = current->pNext;
      free(current->word);
      free(current);
      current = next;
  }
  free(current);
  *pH = NULL;
}

^but I think the problem isn't here.^ Have you any ideas? Could you help me?

[Don't cast malloc](https://stackoverflow.com/q/605845/6699433) — klutt, Jun 03 '20 at 13:59
Do you really have to make a copy within `MakeLowerCase`? If not, then you could do the conversion directly in the passed memory location. — Gerhardh, Jun 03 '20 at 14:04
I will never understand new programmers' fetish for linked lists, you will literally never use them in an actual piece of software because they completely kill your cache, are error prone (as proved over and over) and are large, memory-wise. — Blindy, Jun 03 '20 at 14:04
@Blindy Have you never implemented a linked list when you were a new programmer? :) What you said is true, however they do have useful properties and are warranted in many occasions. — DNT, Jun 03 '20 at 14:08
Can't you change directly via `word` without copying to `lower`? — Gerhardh, Jun 03 '20 at 14:13
Maybe I could, but I need to have original word. I mean I compare it lowercase but in output file it has to be original word. — nbdy, Jun 03 '20 at 14:16
For example: original word: String to compare: string to output file: String — nbdy, Jun 03 '20 at 14:17

DNT · Answer 1 · 2020-06-03T14:02:10.810

0

At the very least, this line

if (*pH == NULL || strcmp(MakeLowerCase((*pH)->word), MakeLowerCase(new_node->word)) == 1)

is continuously leaking memory since MakeLowerCase allocates memory that is never freed.

Consider using something like this

....
char *x1, *x2;
....
// add appropriate NULL checks here before proceeding
x1 = MakeLowerCase((*pH)->word);
x2 = MakeLowerCase(new_node->word);
if (*pH == NULL || strcmp(x1, x2) == 1)
....
free(x1);
free(x2);

It is also a good practice to check whether you actually got the memory you attempted to allocate.

edited Jun 03 '20 at 14:02

answered Jun 03 '20 at 13:57

DNT

2,356
14
16

Hmm how could I fix this? – nbdy Jun 03 '20 at 13:58
1

make variable char* x = MakeLowerCase((*pH)->word), then free it after if? – nbdy Jun 03 '20 at 13:59
Nah, now my program doesn't work. There are no errors during compiling but I have no output file – nbdy Jun 03 '20 at 14:09
@Cris I hope you did not copy my suggestion verbatim right? – DNT Jun 03 '20 at 14:10
Can I answer my my question and share my code or is't prohibited on stackoverflow? – nbdy Jun 03 '20 at 14:12
@Cris, Yes, you can answer your own question. – DNT Jun 03 '20 at 14:13
I did it above, look pls – nbdy Jun 03 '20 at 14:18

score 0 · Answer 2 · answered Jun 03 '20 at 14:15

0

After suggestion of DNT-user I remade the function sortedInsert(). Now it look like this:

void sortedInsert(Word** pH, Word* new_node)
{
    Word* current;
    /* Special case for the head end */
    char* temp_word1 = MakeLowerCase((*pH)->word);
    char* temp_word2 = MakeLowerCase(new_node->word);
    if (*pH == NULL || strcmp(temp_word1, temp_word2 ) == 1)
    {
        new_node->pNext = *pH;
        *pH = new_node;
    }
    else
    {
        /* Locate the node before the point of insertion */
        current = *pH;
        while (current->pNext!=NULL &&
               strcmp(MakeLowerCase(current->pNext->word), MakeLowerCase(new_node->word)) == -1)
        {
            current = current->pNext;
        }
        new_node->pNext = current->pNext;
        current->pNext = new_node;
    }
    free(temp_word1);
    free(temp_word2);
}

There is a fewer memory leaks, but now my program doens't make output file (probably something went wrong).

answered Jun 03 '20 at 14:15

nbdy

65
3

Well, you fixed only one place. There are other places, even in this new function where you still do this `while (current->pNext!=NULL && strcmp(MakeLowerCase(current->pNext->word), MakeLowerCase(new_node->word)) == -1)` which also leaks memory. I suggest you review your entire code carefully. – DNT Jun 03 '20 at 14:20
I know, there are leaks too but why my program stopped working? – nbdy Jun 03 '20 at 14:21
@DNT could we go of any kind of chat like messenger or something? If u have a moment – nbdy Jun 03 '20 at 14:24
You test `if (*pH == NULL `, but now after adding the temp_word code, you do not test before if it is NULL. Check my answer, I have a comment there. You need to restructure your code more to correct the logic and at the same time remove all leaks. – DNT Jun 03 '20 at 14:25
Let me moment I will try to figure it out – nbdy Jun 03 '20 at 14:30
Nah, could you help me little bit? I know the problem is with temp_word1, because it's null at start. I can't handle how to avoid it. Sorry, bad day :v – nbdy Jun 03 '20 at 14:42
Ok, I got this :D I just added simply if(*pH == NULL){*pH = new_node; return; } – nbdy Jun 03 '20 at 15:22

C - Can't get rid of memory leaks

2 Answers2