strtok and storage in arrays: output not as expected

Question

In the below code, the file test.txt has the following data :
192.168.1.1-90
192.168.2.2-80

The output of this is not as expected. I expect the output to be
192.168.1.1
90
192.168.2.2
80
Any help would be much appreciated.

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
int main()
{
  FILE *fp;
  char *result[10][4];
  int i=0;
  const char s[2] = "-";
  char *value,str[128];
  fp = fopen("test.txt", "r");
  if (fp == NULL)
    printf("File doesn't exist\n");
  else{
    while(!feof(fp)){

      if(fgets(str,sizeof(str),fp)){

        /* get the first value */
        value = strtok(str, s);
        result[i][0]=value;
        printf("IP : %s\n",result[i][0]); //to be removed after testing


        /* get second value */
        value = strtok(NULL, s);
        result[i][1]=value;
        printf("PORT : %s\n",result[i][1]); //to be removed after testing
        i++;
     }}
    for (int k=0;k<2;k++){
        for (int j=0;j<2;j++){
            printf("\n%s\n",result[k][j]);
       }
   }

 }
        return(0);
 }

You have the expected output but please also add the actual (wrong) output. — kaylum, Nov 03 '16 at 05:56
The `while` loop is constantly overwriting the same `str` buffer. That's probably not what you want. And also see [Why is “while ( !feof (file) )” always wrong?](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) — kaylum, Nov 03 '16 at 05:57
If doing POSIX, replace `strtok(str, s)` by `strdup(strtok(str, s))`. Also take care to free `result[i][x]` if not used anymore than. — alk, Nov 03 '16 at 06:03
A string literal is fine for the delimiter, e.g. `const char *s = "-";` Your character array is fine as well. — David C. Rankin, Nov 03 '16 at 06:03
OT: The common notation to describe addresses with ports is "*address:port*", note the colon. — alk, Nov 03 '16 at 06:05
@kaylum the actual output is : 192.168.2.2 80 192.168.2.2 80 And the `str` , its value is being written in the array and then its being overwritten. Then how is it creating a problem.? — vatsal511, Nov 03 '16 at 06:22
You are storing *pointers* to str in the array. You are not making copies of the content. — kaylum, Nov 03 '16 at 06:26
So what must I change, I'm all puzzled up in here... @kaylum — vatsal511, Nov 03 '16 at 06:32

score 0 · Answer 1 · answered Nov 03 '16 at 07:04

You can try this solution. It uses dynamic memory instead, but does what your after.

The code:

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

#define BUFFSIZE 128

void exit_if_null(void *ptr, const char *msg);

int
main(int argc, char const *argv[]) {
    FILE *filename;
    char buffer[BUFFSIZE];
    char *sequence;
    char **ipinfo;
    int str_size = 10, str_count = 0, i;

    filename = fopen("ips.txt", "r");

    if (filename == NULL) {
        fprintf(stderr, "%s\n", "Error Reading File!");
        exit(EXIT_FAILURE);
    }

    ipinfo = malloc(str_size * sizeof(*ipinfo));
    exit_if_null(ipinfo, "Initial Allocation");

    while (fgets(buffer, BUFFSIZE, filename) != NULL) {
        sequence = strtok(buffer, "-\n");

        while (sequence != NULL) {
            if (str_size == str_count) {
                str_size *= 2;
                ipinfo = realloc(ipinfo, str_size * sizeof(*ipinfo));
                exit_if_null(ipinfo, "Reallocation");
            }

            ipinfo[str_count] = malloc(strlen(sequence)+1);
            exit_if_null(ipinfo[str_count], "Initial Allocation");

            strcpy(ipinfo[str_count], sequence);

            str_count++;

            sequence = strtok(NULL, "-\n");
        }
    }

    for (i = 0; i < str_count; i++) {
        printf("%s\n", ipinfo[i]);
        free(ipinfo[i]);
        ipinfo[i] = NULL;
    }

    free(ipinfo);
    ipinfo = NULL;

    fclose(filename);

    return 0;
}

void
exit_if_null(void *ptr, const char *msg) {
    if (!ptr) {
        printf("Unexpected null pointer: %s\n", msg);
        exit(EXIT_FAILURE);
    }
}

score 0 · Answer 2 · answered Nov 03 '16 at 10:08

The key thing to understand is that char *strtok(char *str, const char *delim) internally modifies the string pointed to by str and uses that to store the result. So the returned pointer actually points to somewhere in str.

In your code, the content of str is refreshed each time when you parse a new line in the file, but the address remains the same. So after your while loop, the content of str is the last line of the file, somehow modified by strtok. At this time, result[0][0] and result[1][0] both points to the same address, which equals the beginning of str. So you print the same thing twice in the end.

This is further illustrated in the comments added to your code.

int main()
{
    FILE *fp;
    char *result[10][4];
    int i=0;
    const char s[2] = "-";
    char *value,str[128];
    fp = fopen("test.txt", "r");
    if (fp == NULL)
        printf("File doesn't exist\n");
    else{
        while(!feof(fp)){

            if(fgets(str,sizeof(str),fp)){

                /* get the first value */
                value = strtok(str, s);
                // ADDED: value now points to somewhere in str
                result[i][0]=value;
                // ADDED: result[i][0] points to the same address for i = 0 and 1
                printf("IP : %s\n",result[i][0]); //to be removed after testing


                /* get second value */
                value = strtok(NULL, s);
                // ADDED: value now points to somewhere in str
                result[i][1]=value;
                // ADDED: result[i][1] points to the same address for i = 0 and 1
                printf("PORT : %s\n",result[i][1]); //to be removed after testing
                i++;
            }}
        // ADDED: now result[0][0]==result[1][0], result[0][1]==result[1][1], you can test that
        for (int k=0;k<2;k++){
            for (int j=0;j<2;j++){
                printf("\n%s\n",result[k][j]);
            }
        }

    }
    return(0);
}

To get the expected output, you should copy the string pointed by the pointer returned by strtok to somewhere else each time, rather than just copy the pointer itself.

strtok and storage in arrays: output not as expected

2 Answers2