Seg fault error?

Question

Does anyone know why I keep getting this seg fault error? I am trying to run a program that locates a substring "from" within "src" and replaces all non-overlapping occurrences of "from" in src with "to" in the output string "dest". Also, could anyone provide me with the proper way to test this case? As I'm not too sure how I could display this with type "void"... (Trying this as an exercise)

void find_replace(char* src, char* from, char* to, char* dest)
{
  int count = 0;
  int diff = strlen(to) - strlen(from);
  int destlength = strlen(src);
   dest = malloc(destlength);
  for (int i = 0; i < strlen(src); i++)
    {
      int index = 0;
      while (src[i+index] == from[index] && index < strlen(from)){
        index++;
      }
      if (index == strlen(from)) {
        for (int j = 0; j < strlen(to); j++) {
          dest[i+j+(count * diff)] = to[j];
        }
        i += strlen(from) - 1;
        count++;
      }
      else {
        dest[i + (count * diff)] = src[i];
      }
    }
  return ; 
}

Is it sufficient enough to do this for a test?

int main (int argc, char *argv[])
{
     char* dest;
   find_replace("hello my name is leeho lim", "leeho lim", "(insert name)" dest);
  for (int i = 0; i < strlen(dest); i++)
    {
      printf("%c", dest[i]);
    }
  printf("\n");
}

Answer 1

The problems happens because you are trying to access an unallocated pointer with

strlen(dest)

In the for in your main program.

The reason for this is that you sent the value of the pointer dest to the function, not the pointer itself, so when you allocated the memory inside your function, so you didn't actually modify the memory address stored in the pointer outside of it.

When you send the pointer as a parameter of a function, what you are actually doing is sending the memory address stored in that pointer, you are sending the value stored in the pointer, not the pointer itself.

If you want to get the allocated memory address for the string, you either can make the function return it, or you can declare and send dest as a pointer to a pointer.

EDIT: As the other comment points out, you can also perform the allocation in main(), instead of doing it inside your function.

Answer 2

A few minor tweaks to your program is all you need.

Change the return value of find_replace to be the newly allocated memory for the changed string.

Instead of

void  find_replace(char* src, char* from, char* to, char* dest)

Use

char* find_replace(char* src, char* from, char* to)

Change the implementation slightly.

Instead of

  dest = malloc(destlength);

use

  char* dest = malloc(destlength);

and

Instead of

  return;

use

 return dest;

Change the way you use the function.

Instead of

char* dest;
find_replace("hello my name is leeho lim", "leeho lim", "(insert name)", dest);

use

char* dest = find_replace("hello my name is leeho lim", "leeho lim", "(insert name)");

Make sure to deallocate memory returned by find_replace.

free(dest);

Here's a fully working program:

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

char* find_replace(char* src, char* from, char* to)
{
  int count = 0;
  int diff = strlen(to) - strlen(from);
  int destlength = strlen(src);
  char* dest = malloc(destlength);
  for (int i = 0; i < strlen(src); i++)
    {
      int index = 0;
      while (src[i+index] == from[index] && index < strlen(from)){
        index++;
      }
      if (index == strlen(from)) {
        for (int j = 0; j < strlen(to); j++) {
          dest[i+j+(count * diff)] = to[j];
        }
        i += strlen(from) - 1;
        count++;
      }
      else {
        dest[i + (count * diff)] = src[i];
      }
    }
  return dest; 
}

int main (int argc, char *argv[])
{
   char* dest = find_replace("hello my name is leeho lim", "leeho lim", "r sahu");
   for (int i = 0; i < strlen(dest); i++)
   {
      printf("%c", dest[i]);
   }
   free(dest);
   printf("\n");
}

Update, in response to OP's comment

If the return type of find_replace must be void, there are couple of options on how to deal with the memory needed for dest.

1. Allocate memory for dest in the calling function.

   char* dest = malloc(SUFFICIENT_SIZE);
   find_replace("hello my name is leeho lim", "leeho lim", "(insert name)", dest); 
   
   free(dest);
   

   Then, there is no need for the line

   dest = malloc(...); 
   

   in find_replace.

2. Allocate memory for dest in find_replace. Then, you need to pass a pointer to a pointer.

   void find_replace(char* src, char* from, char* to, char** dest)
   

   and use *dest instead of just dest in the function.

   void find_replace(char* src, char* from, char* to, char** dest)
   {
     int count = 0;
     int diff = strlen(to) - strlen(from);
     int destlength = strlen(src);
     *dest = malloc(destlength);
     for (int i = 0; i < strlen(src); i++)
       {
         int index = 0;
         while (src[i+index] == from[index] && index < strlen(from)){
           index++;
         }
         if (index == strlen(from)) {
           for (int j = 0; j < strlen(to); j++) {
             (*dest)[i+j+(count * diff)] = to[j];
           }
           i += strlen(from) - 1;
           count++;
         }
         else {
           (*dest)[i + (count * diff)] = src[i];
         }
       }
     return; 
   }
   

   and change the you call find_replace.

   char* dest;
   find_replace("hello my name is leeho lim", "leeho lim", "(insert name)", &dest);
   

Answer 3

There are two main problems with your (original) code:

1. it does not provide for your find_replace() function to return the allocated destination buffer to the caller, and
2. it does not reliably allocate enough space for the destination buffer.

In principle, issue (1) could be resolved in two ways. Either the space could be allocated by the caller and a pointer to it passed to the function, or the space could be allocated by the function and a pointer returned to the caller. Your original code allocates space in the function, but does not return a pointer to it to the caller.

It is preferable for the function to perform the allocation, because satisfying issue (2) requires a more thorough analysis of the inputs than makes sense to insist the caller perform. Consider what happens in your revised code when you do this:

char dest[4];
int canary = 0;

find_replace("aaa", "a", "longer string", dest);
assert(canary == 0);

Most likely you get a segfault, possibly you get an assertion failure, and perhaps you get who-knows-what, because find_replace() cannot perform its job without writing past the end of dest, the result of which is undefined.

Although you've said the exercise requires your function to have no return value (i.e. void), it can still return a pointer to the destination string via the argument list. You simply pass a pointer to the dest pointer instead of its value, so that the function can update that value. The signature would look like this:

void find_replace(const char *src, const char *from, const char *to,
                  char **dest_p);

(Note the const qualifers for src, from, and to, which are appropriate, if not necessary, if the function is meant to accept string literals.)

The amount of space needed for dest is the length of src plus one for the terminator plus, if to is longer than from, the difference between the lengths of the to and from strings times the number of appearances of the to string. You could, however, compute an upper bound on that length, and later shrink the allocation (if needed) after you find out how much space is actually used. For example:

void find_replace(const char *src, const char *from, const char *to,
                  char **dest_p) {
    ssize_t src_size = strlen(src);
    ssize_t from_size = strlen(from);
    ssize_t to_size = strlen(to);
    char *temp;

    if (!from_size) {
         /* special case: the 'from' string is empty ... */
         /* whatever you do, set temp to something valid or NULL */
    } else {
        ssize_t size_diff = to_size - from_size;

        if (size_diff < 0) size_diff = 0;
        temp = malloc(1 + src_size + (src_size / from_size) * size_diff);
        if (temp) {
            /* use next to track the next unused position in temp */
            char *next = temp;

            /*
             * perform the substitution, updating 'next' appropriately as
             * you go along (INSERT CODE AFTER THIS COMMENT) ...
             */

            /* be sure to terminate: */
            *(next++) = '\0';

            /* shrink the string to the actual space used (optional): */
            next = realloc(temp, next - temp);

            /*
             * On (re)allocation error, next will be NULL and temp will still
             * be a valid pointer.  Otherwise, next will be a pointer to the
             * space, not necessarily equal to temp, and temp might no longer
             * be a valid pointer.
             *
             * An OK error recovery strategy is to just return a pointer
             * to the full-size space.
             */
            if (next) {
                temp = next;
            }
        } /* else allocation failure; return NULL */
    }

    /*
     * The caller gets a pointer to the allocated space (if any).  It is his
     * responsibility to free it when it is no longer needed.
     */
    *dest = temp;
}

The actual substitution code is left as an exercise, since this is homework, after all.