C - What's wrong with mystrtok() function?

Question

I'm trying to create a function that'll do the same work as strtok() function does in C. Below is my code, but the problem is that every time I run this program, it shows only the first tokenized string, then the program stops and Windows shows a pop-up saying "..... has stopped working......" & then then Code::Blocks returns -1073741819 (0XC0000005). I don't know why this is happening — can you explain?

My code:

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

char a[1000],b[50];
int i=0;

char *mystrtok(char a[],char b[])
{
    char c[1000];
    char *ptr = malloc(1000*sizeof(c));
    ptr = c;
    c[0] = '\0';

    if(a == NULL)
        ++i;
    else
        i=0;

    /*printf("i is = %d\n",i);
    free(ptr);*/

    //printf("len of c is%d",strlen(c));
    int j=0,flag;

    //ptr = realloc(ptr,sizeof(c));
    for(i=i;a[i] != '\0';i++)
    {
        for(j=0;b[j] != '\0';j++)
        {
            if(a[i] == b[j])
            {
                c[i] = '\0';
                return ptr;
            }
            else
            {
                flag = 0;
                c[i] = a[i];
                continue;
            }
        }
    }
    /*if(!flag)
        c[i] = '\0';*/
    return ptr;
}

int main()
{
    int k;
    printf("Enter a big string: ");
    gets(a);
    printf("Enter a token: ");
    gets(b);

    char *tokenized;
    tokenized = mystrtok(a,b);
    puts(tokenized);

    while(tokenized)
    {
        tokenized = mystrtok(NULL, b);
        puts(tokenized);
    }
}

I've spent much time on finding what's problem with my code, and I've searched through Google & Stack Overflow help, but nothing helps me exactly.

Fakruddin Gazzali The standard function strtok does not allocate memory. Moreover your implementation has a memory leak char *ptr = malloc(1000*sizeof(c)); ptr = c; That is the address of the allocated memory is lost because the pointer is reassigned. — Vlad from Moscow, Jul 09 '17 at 13:31
should I remove it? But is remove it, how can I get back the pointer pointing to that string c?Yesterday I read that if I want to a function to return string , then I should use `malloc()` and I should assign a pointer to that string? Is this wrong? I'm sorry that I don't have much knowledge about pointers — Md. Fakruddin Gazzali Fahim, Jul 09 '17 at 13:38
To rewrite `strtok()` you need a `static` variable to keep status inside the function. But maybe you want a well behaved (different) implementation??? — pmg, Jul 09 '17 at 13:44
You understand that the 'standard' strtok() is broken? It uses static state, and so is not reeentrant and thread-unsafe. — ThingyWotsit, Jul 09 '17 at 13:45
@Md.FakruddinGazzaliFahim Yes you should remove the allocation because in any case the memory is not used, its address is lost. Also it is a bad idea that the function uses the global variable i. — Vlad from Moscow, Jul 09 '17 at 13:46
@VladfromMoscow okay, trying that with internal variable ... But I should store the value of i for the next run (i.e. when I'm calling ` tokenized = mystrtok(NULL, b);` . So I thought in lobal variable might help me ! — Md. Fakruddin Gazzali Fahim, Jul 09 '17 at 13:49
Question: in the 2nd (and later) call to `strtok()` how does it know where to look for the separators (remember you pass `NULL` fir the 1st parameter)? Answer: it keeps state in `static` objects. — pmg, Jul 09 '17 at 13:52
@Md.FakruddinGazzaliFahim You should declare a static variable inside the function. It is better to declare it of the type char *. — Vlad from Moscow, Jul 09 '17 at 13:54
@Md.FakruddinGazzaliFahim Pay attention to this for statement for(i=i;a[i] != '\0';i++) This expression i = i; within it does not make sense. — Vlad from Moscow, Jul 09 '17 at 13:59
Note that you are allocating one megabyte of memory, which seems like overkill. — Jonathan Leffler, Jul 09 '17 at 14:19
Also note that [`gets()` is too dangerous to be used, ever!](http://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used) — Jonathan Leffler, Jul 09 '17 at 15:33

kamoroso94 · Answer 1 · 2017-07-09T15:05:49.943

You have a few bugs. Right off the top, you allocate memory for ptr, then lose the reference to it by overwriting it with c. That's a memory leak (and also because you never free it).

The main problem I think is that when the second call passes NULL, you lost the reference to the string you're supposed to be searching from the previous call. You have to have some memory that persists between function calls. static variables are perfect for this.

And lastly, the loop in main has the statements in the reverse order.

#include <stdio.h>

char *mystrtok(char *str, const char *delim) {
  int i = 0, j;
  static char *ptr = "";
  int found;

  // new str, new search
  if(str != NULL) {
    ptr = str;
  }

  // skip delims
  while(ptr[i] != '\0') {
    found = 1;
    for(j = 0; delim[j] != '\0'; j++) {
      if(ptr[i] == delim[j]) {
        found = 0;
        break;
      }
    }

    if(found) {
      break;
    }

    i++;
  }

  if(ptr[i] == '\0') {
    return NULL;
  } else {
    ptr += i;
  }

  // find next delim
  found = 0;
  while(ptr[i] != '\0') {
    for(j = 0; delim[j] != '\0'; j++) {
      if(ptr[i] == delim[j]) {
        ptr[i] = '\0';
        found = 1;
        break;
      }
    }

    if(found) {
      break;
    }

    i++;
  }

  // record start, advance ptr
  str = ptr;
  ptr += i;

  // only skip delims
  if(found) {
    ptr++;
  }

  return str;
}

int main() {
  char a[1000];
  char b[50];

  printf("Enter a big string: ");
  gets(a);

  printf("Enter a token: ");
  gets(b);

  char *tokenized = mystrtok(a, b);

  while(tokenized != NULL) {
    puts(tokenized);
    tokenized = mystrtok(NULL, b);
  }

  return 0;
}

C - What's wrong with mystrtok() function?

1 Answers1