CODEWITHSUNDEEP
c
user1089679
user1089679

Reputation: 2368

Getting Segmentation Fault in Realloc

Here i want to create dynamically memory. here i dnt know the output size and i want to print last final output after while loop.

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

void main() {

    char *sdpCommand = "sdptool browse 04:18:0F:B1:48:B5";

    FILE *fp;
    fp = popen(sdpCommand, "r");

    char *results = 0;
    if (fp == NULL) {
        printf("Failed to run command\n");
        return;
    }

    char* buffer = malloc(514);
    while (fgets(buffer, strlen(buffer) - 1, fp) != NULL) {
        realloc(results, strlen(results) + strlen(buffer));
        memcpy(results + strlen(results), buffer, strlen(buffer));
    }
    printf("Output    :::  %s", results);

    /* close */
    pclose(fp);
    sleep(1);

}

Upvotes: 7

Views: 16854

Answers (3)

NPE
NPE

Reputation: 500187

There are two main issues:

  1. realloc() returns the new address:

    new_results = realloc(results, ...);
if (new_results != NULL) {
  results = new_results;
} else {
  /* handle reallocation failure, `results' is still valid */
}

  2. sizeof() is not the right way to find out the size of results and of buffer. It would simply return the size of the pointer. For results, you probably want to keep track of the allocated size yourself. For buffer, you're probably looking for strlen().

Once you fix the above, you need to make sure that results ends up as a valid NUL-terminated string. This is necessary for the printf() to work correctly.

Upvotes: 14

whtlnv
whtlnv

Reputation: 2207

realloc(results, sizeof(results) + sizeof(buffer));

This line could be the problem.

  1. realloc() returns the new address allocated with the specified size, and there's no guarantee that it will be the same address the pointer had before; So, you should always reassign your pointer:

    ptr = realloc( ptr, newSize );

  2. sizeof() will not return the dynamic allocated size of a pointer. In this case sizeof(results) and sizeof(buffer) are most probably returning always 8. You might replace with:

    results = realloc(results, ctr * 514);
    Where ctr is a short initialized with 0 before the while and counting the number of iterations.

  3. Since buffer is always 514 why don't you reduce the load by redifining it to:

    char buffer[514];

    Best Regards

Upvotes: 2

unwind
unwind

Reputation: 399743

Your code is just blindly forging ahead, seemingly written according to the "it compiles, it must be working" paradigm. I'm sorry if that sounds harsh, it's not a comment about you personally of course.

You need to really look at the documentation for the API:s you're trying to use, and analyze whether or not your usage makes sense.

You can't ignore the return value from realloc(), how would you get the newly allocated memory? It can't be changing your existing pointer, since you're passing it by value.

The way a dynamic array generally needs to work is to keep track of three things:

  • The current array's base address
  • The number of elements (bytes, in your case) that the array holds right now
  • The number of elements that the array can hold, maximum (before it needs to grow)

When appending n new elements, you check if doing so overflows the number of elements, so it becomes larger than the max. That is not allowed, so in that case you need to re-allocate the base array so the new data fits, and (of course) update the array's state accordingly.

Upvotes: 3

Related Questions