realloc: invalid next size and malloc: memory corruption (fast)

Question

I am doing an exercise for fun from K and R C programming book. The program is for finding the longest line from a set of lines entered by the user and then prints it.

Inputs:

This is a test

This is another long test

this is another long testthis is another long test

Observation:

It runs fine for the first two inputs but fails for the larger string (3rd input)

Errors:

Error in `./longest': realloc(): invalid next size: 0x000000000246e010 ***
Error in `./longest': malloc(): memory corruption (fast): 0x000000000246e030 ***

My efforts:

I have been trying to debug this since 2 days (rubber duck debugging) but the logic seems fine. GDB points to the realloc call in the _getline function and shows a huge backtrace with glibc.so memory allocation calls at the top.

Here is what I have written (partially, some part is taken from the book directly):-

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

int MAXLINE =  10;
int INCREMENT = 10;

char* line = NULL, *longest = NULL;

void _memcleanup(){
    free(line);
    free(longest);      
}

void copy(char longest[], char line[]){
    int i=0;
    char* temp = realloc(longest,(MAXLINE)*sizeof(char));
    if(temp == NULL){
        printf("%s","Unable to allocate memory");
        _memcleanup();
        exit(1);
    }
    longest = temp;

    while((longest[i] = line[i]) != '\0'){
        ++i;
    }    
}

int _getline(char s[]){
  int i,c;
  for(i=0; ((c=getchar())!=EOF && c!='\n'); i++){
    if(i == MAXLINE - 1){
      char* temp = realloc(s,(MAXLINE + INCREMENT)*sizeof(char));
      if(temp == NULL){
        printf("%s","Unable to allocate memory");
        _memcleanup();
        exit(1);
      }

      s= temp;
      MAXLINE += INCREMENT;
    }
    s[i] = c;
  }

  if(c == '\n'){
      s[i++] = c;
  }

  s[i]= '\0';

  return i;
} 

int main(){
  int max=0, len;

  line = malloc(MAXLINE*sizeof(char));
  longest = malloc(MAXLINE*sizeof(char));

  while((len = _getline(line)) > 0){
    printf("%d%d", len, MAXLINE);
    if(len > max){
      max = len;
      copy(longest, line);
    }
  }
  if(max>0){
    printf("%s",longest);
  }

  _memcleanup();
  return 0;
}

When `i == MAXLINE - 2`, no realloc occurs. If the next `getchar()`, now with `i == MAXLINE - 1`, is a `'\n'`, loop ends and `s[MAXLINE - 1] = '\n';`. _Then_ `s[MAXLINE]= '\0';`, which is UB. — chux - Reinstate Monica, Jul 06 '14 at 05:00
possible duplicate of [realloc() invalid old size](http://stackoverflow.com/questions/24579698/realloc-invalid-old-size) — Bill Lynch, Jul 06 '14 at 06:13
On a side note, I spent about 30 seconds thinking that `MAXLINE` was a constant. Best to not name variables like constants or macros. — Ed S., Jul 06 '14 at 06:21

score 5 · Accepted Answer · answered Jul 06 '14 at 04:44

You´re reallocating on copied addresses (because parameters).
A parameter in C is a copy of the original value everytime; in case of
a pointer it will point to the same location but the address itself is copied.

realloc resizes the buffer asociated with the address, everything fine so far.
But it can relocate the whole thing and assign a completely new address,
and this new address (if it happens) will be lost after the function returns to main.

Use a double pointer:
Pass a char **s instead of char *s (==char s[]) as formal parameter,
pass &xyz intead of xyz as actual value, and inside the function,
use *xyz and **xyz (or (*xyz)[index]) for address and value.

Other things:
Global variables are ugly (and confusing when named same as parameters),
multiplying with sizeof(char) is nonsense because it´s be 1 everytime,
and names in capitals should be used for #define´s rather than variables.

How do i avoid ugly global variables? My situation is that i need to call free from multiple functions on the same set of pointers. — user720694, Jul 06 '14 at 08:05
Well, make the variable in main and pass it as parameter. If you include the double pointer stuff and add (poimter) to int for the to numbers, copy is already OK... — deviantfan, Jul 06 '14 at 08:46

David C. Rankin · Answer 2 · 2014-07-06T07:59:37.730

The double pointer alone, isn't the solution to your problems. You have 2 primary issues. You can see them by entering your strings as a string of characters and will notice you problem occurs when you pass the 20th character. (e.g. 01234567890123456789)

You have declared both line and longest globally. So while you can rewrite _getline (char **s), you can also simply update line at the end of _getline with memcpy (include string.h). For example:

    memcpy (line, s, (size_t)i);

    return i;
}

That cures your _getline issue. Issue two is fairly straight forward. You are not null-terminating longest in copy. (your choice of arguments with the same name as the globals presents challenges as well) Including the following fixes copy:

        ++i;
    }

    longest[i] = '\0';
}

If you incorporate both changes, then I believe you will find you routine works. You can then rewite _getline (char **s) and pass &line as another exercise. For example, you can rewrite _getline as:

int
_getline (char **s) {
    int i, c;
    for (i = 0; ((c = getchar ()) != EOF && c != '\n'); i++) {
        if (i == MAXLINE - 1) {
            char *temp = realloc (*s, (MAXLINE + INCREMENT) * sizeof (char));
            if (temp == NULL) {
                printf ("%s", "Unable to allocate memory");
                _memcleanup ();
                exit (1);
            }
            *s = temp;
            MAXLINE += INCREMENT;
        }
        (*s)[i] = c;
    }

    if (c == '\n') {
        (*s)[i++] = c;
    }

    (*s)[i] = '\0';

    return i;
}

And then modify your call in main to:

while ((len = _getline (&line)) > 0) {

Thanks for the thanks. I knew you had a headache with this one because I nearly pulled my hair out as well :) — David C. Rankin, Jul 06 '14 at 08:25

realloc: invalid next size and malloc: memory corruption (fast)

Inputs:

Observation:

Errors:

My efforts:

2 Answers2

Linked