Using gdb--still can't find malloc error

Question

I've looked at previous posts about this and they didn't help me locate my problem... To keep it short I'm making a function should read a text file line by line (and yes, I do realize there are many posts like this). But when I run my program through CMD, it's giving me this error:

Program received signal SIGSEGV, Segmentation fault.
__GI___libc_realloc (oldmem=0x10011, bytes=1) at malloc.c:2999
2999    malloc.c: No such file or directory.

I'm pretty sure I wrote out my malloc/realloc lines correctly. I've tried finding alot of posts similar to this, but none of the solutions offered are helping. If you have any post suggestions that maybe I missed, please let me know. Regardless, here are my functions:

char* read_single_line(FILE* fp){
  char* line = NULL;
  int num_chars = 0;
  char c;
  fscanf(fp, "%c", &c);
  while(!feof(fp)) {
    num_chars++;
    line = (char*) realloc(line, num_chars * sizeof(char));
    line[num_chars -1] = c;
    if (c == '\n') {
      break;
    }
    fscanf(fp, "%c", &c);
  }
  if(line != NULL) {
    line = realloc(line, (num_chars+1) * sizeof(char));
    line[num_chars] = '\0';
  }
  return line;
}


void read_lines(FILE* fp, char*** lines, int* num_lines) {
  int i = 0;
  int num_lines_in_file = 0;
  char line[1000];
  if (fp == NULL) {
    *lines = NULL;
    *num_lines = 0;
  } else {
    (*lines) = (char**)malloc(1 * sizeof(char*));
    while (read_single_line(fp) != NULL) {
      (*lines)[i] = (char*)realloc((*lines)[i], sizeof(char));
      num_lines_in_file++;
      i++;
    }
    *lines[i] = line;
    *num_lines = num_lines_in_file;

  }
}

I would really appreciate any help--I'm a beginner in C so hear me out!!

Quite often a segmentation error in malloc is a result of a prior storage overlay, not because of bad parameters passed to malloc. Please provide [MVCE](https://stackoverflow.com/help/mcve) — TonyB, Nov 30 '17 at 05:42
Compile with all warnings and debug info: `gcc -Wall -Wextra -g` with [GCC](http://gcc.gnu.org/). Use not only `gdb` but also [valgrind](http://valgrind.org/) — Basile Starynkevitch, Nov 30 '17 at 06:00
BTW, why don't you use [getline](https://sourceware.org/gdb/current/onlinedocs/gdb/) instead of having your (inefficient) `read_single_line` ? — Basile Starynkevitch, Nov 30 '17 at 06:02
Usually when using gdb to debug a crash, it is quite useful to look a the [back trace](http://ftp.gnu.org/old-gnu/Manuals/gdb/html_node/gdb_42.html) and move throught the call stack. — dvhh, Nov 30 '17 at 06:20
You're also [not using `realloc` correctly](https://stackoverflow.com/questions/21006707/proper-usage-of-realloc) — Antti Haapala -- Слава Україні, Nov 30 '17 at 07:14
Also [don't cast the return value of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Antti Haapala -- Слава Україні, Nov 30 '17 at 07:15
Also one can tell that you're beginner from the [use of 3 stars in a type](http://wiki.c2.com/?ThreeStarProgrammer). — Antti Haapala -- Слава Україні, Nov 30 '17 at 07:19
@AnttiHaapala: No need for mean-spirited comments. It turns out the `char ***` is a valid out-variable for the array of `char*`s allocated within the function. I have found 1 or 2 uses for such types over the decades. — lockcmpxchg8b, Nov 30 '17 at 07:27
@lockcmpxchg8b It is a valid type, but even this wasn't a valid use case. return value from the function was `void` - it could have returned the `char **` instead. — Antti Haapala -- Слава Україні, Nov 30 '17 at 08:13
Sure...until he implements error checking. Between you and I we probably have ~50 years of programming experience to see such nuance; the OP is just learning. — lockcmpxchg8b, Nov 30 '17 at 08:29

paxdiablo · Answer 1 · 2017-11-30T06:39:51.333

char line[1000];
:
while (read_single_line(fp) != NULL) {
:
}
*lines[i] = line;

This doesn't look at all right to me. Your read_single_line function returns an actual line but, other than checking that against NULL, you never actually store it anywhere. Instead, you point the line pointer to line, a auto-scoped variable which could contain literally anything (and, more worrying, possibly no terminator character).

I think you should probably store the return value from read_single_line and use that to set your line pointers.

By the way, it may also be quite inefficient to expand your buffer one character at a time. I'd suggest initially allocating more bytes and then keeping both that capacity and the bytes currently in use. Then, only when you're about to use beyond your capacity do you expand, and by more than one. In pseudo-code, something like:

def getLine:
    # Initial allocation with error check.

    capacity = 64
    inUse = 0
    buffer = allocate(capacity)
    if buffer == null:
        return null

    # Process each character made available somehow.

    while ch = getNextChar:
        # Expand buffer if needed, always have room for terminator.

        if inUse + 1 == capacity:
            capacity += 64
            newBuff = realloc buffer with capacity

            # Failure means we have to release old buffer.

            if newBuff == null:
                free buffer
                return null

        # Store character in buffer, we have enough room.

        buffer[inUse++] = ch

    # Store terminator, we'll always have room.

    buffer[inUse] = '\0';
    return buffer

You'll notice, as well as the more efficient re-allocations, better error checking on said allocations.

n. m. could be an AI · Accepted Answer · 2017-12-01T19:09:12.477

while (read_single_line(fp) != NULL) {
      (*lines)[i] = (char*)realloc((*lines)[i], sizeof(char));
      num_lines_in_file++;
      i++;
    }
    *lines[i] = line;

There are more errors then lines in this short fragment. Let's go over them one by one.

 while (read_single_line(fp) != NULL)

You read a line, check whether it's a null pointer, and throw it away instead of keeping it around to accumulate it in the lines array.

   (*lines)[i] = (char*)realloc((*lines)[i], sizeof(char));

You are trying to realloc (*lines[i]). First, it does not exist beyond i==0, because (*lines) was only ever allocated to contain one element. Second, it makes no sense to realloc individual lines, because you are (should be) getting perfect ready-made lines from the line reading function. You want to realloc *lines instead:

*lines = realloc (*lines, i * sizeof(char*));

Now these two lines

  num_lines_in_file++;
  i++;

are not an error per se, but why have two variables which always have the exact same value? In addition, you want them (it) be before the realloc line, per usual increment-realloc-assign pattern (you are using it in the other function).

Speaking of the assign part, there isn't any. You should insert one now:

(*lines)[i-1] = // what?

The line pointer you should have saved when calling read_single_line, that's what. From the beginning:

char* cur_line;
int i = 0;
*lines = NULL;
while ((cur_line=read_single_line(fp)) != NULL) 
{
   ++i;
   *lines = realloc (*lines, i * sizeof(char*));
   (*lines)[i-1] = cur_line;
}
*num_lines = i;

The last one

   *lines[i] = line;

is downright ugly.

First, lines is not an array, it's a pointer pointing to a single variable, so lines[i] accesses intergalactic dust. Second, you are trying to assign it an address of a local variable, which will cease to exist as soon as your function returns. Third, what is it doing outside of the loop? If you want to terminate your line array with a null pointer, do so:

} 
*num_lines = i;
++i;
*lines = realloc (*lines, i * sizeof(char*));
(*lines)[i-1] = NULL;

But given that you return the number of lines, this may not be necessary.

Disclaimer: none of the above is tested. If there are any bugs, fix them!

Using gdb--still can't find malloc error

2 Answers2