1

I have a file like this

GET /index.html k
GET /docencia.html k
GET /ejemplo.html k

and I want to read it line by line and split it up with this delimiter " " but is giving me this error: segmentation fault(core dumped) and I don't know what to try.

This is my code:

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

int main(int argc, char *argv[]) {
    char ordenes[150];
    char *orden[3];
    char *token;
    int tok;

    FILE *fp;
    fp = fopen(argv[1], "r");
    if (fp == NULL) {
        printf("File error");
        exit(1);
    }

    while (feof(fp) == 0) {
        fgets(ordenes, sizeof(ordenes), fp);
        printf("%s \n", ordenes);
        token = strtok(ordenes, " ");
        tok = 0;
        while (token != NULL) {
            orden[tok] = strdup(token);
            tok++;
            token = strtok(NULL, " ");
        }

        printf("\n%s\n", orden[0]);
        printf("\n%s\n", orden[1]);
        printf("\n%s\n", orden[2]);
    }
    fclose(fp);
}

The error shows when I call the first strdup. If I try to print the token just after I call the first strtok, it fails too (the same segmentation fault core dumped) so I guess the problem is with the strtok.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • `token = strtok(NULL," ");` move to loop-end. or `if(token)break;` after `token = strtok(NULL," ");`. – BLUEPIXY Dec 10 '16 at 12:21
  • 1
    put `#include `. – BLUEPIXY Dec 10 '16 at 12:30
  • Okay the include was the problem... The compiler didnt show anything so i thought with the std was good. –  Dec 10 '16 at 12:30
  • 1
    Please read ["why is `while(!feof())` always wrong"](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) too. – unwind Dec 10 '16 at 12:51

5 Answers5

4

You do not include <string.h>, so the compiler applies the default argument promotions on the signature of strtok, in particular it considers that strtok returns an int.

So the compiler will apply an operator of coercion from int to pointer to char at the assignment

token = strtok(ordenes, " ");

and this assignment will be compiled as

token = (int->char*) strtok(ordenes, " ");
alinsoar
  • 15,386
  • 4
  • 57
  • 74
  • Good catch! This will definitely cause problems on 64-bit systems, while is might go unnoticed on 32-bit ones. Definitely undefined behavior! You could also advise the OP to use compiler warnings to avoid such silly mistakes. `gcc -Wall -W`, `clang -Weverything`... – chqrlie Dec 10 '16 at 15:02
2

There are multiple problems in your code:

  • As alinsoar diagnosed with a sharp eye, you do not include <string.h>. strtok is not defined, the compiler must assume it returns an int, which it does not, and this int is silently converted to a char *. The code generated invokes undefined behavior and will most likely crash on 64-bit targets. You should compile with all warnings enabled to let the compiler help avoid this kind of silly mistake. gcc -Wall -W or clang -Weverything...

  • You do not check if command line arguments have been passed to your program before calling fp = fopen(argv[1], "r");. If no arguments are passed, argv[1] is a null pointer.

  • while (feof(fp) == 0) is incorrect, read Why is “while ( !feof (file) )” always wrong? . You should instead write while (fgets(ordenes, sizeof(ordenes), fp)) {...

  • You do not check if tok < 3 before storing token into the orden array. If the line has more than 3 fields, you will cause a buffer overflow.

  • You do not check if 3 tokens were indeed found before printing all 3 entries in orden. This too might invoke undefined behavior, especially if fgets() failed to read a line, which you do not check.

Here is an improved version:

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

int main(int argc, char *argv[]) {
    char ordenes[150];
    char *orden[3];
    char *token;
    int i, tok;

    if (argc < 2) {
        fprintf(stderr, "Missing command line argument\n");
        exit(1);
    }
    FILE *fp = fopen(argv[1], "r");
    if (fp == NULL) {
        fprintf(stderr, "Cannot open input file %s: %s\n", 
                argv[1], strerror(errno));
        exit(1);
    }

    while (fgets(ordenes, sizeof(ordenes), fp)) {
        printf("%s", ordenes);
        token = strtok(ordenes, " ");
        for (tok = 0; tok < 3 && token != NULL; tok++) {
            orden[tok] = strdup(token);
            token = strtok(NULL, " ");
        }
        for (i = 0; i < tok; i++) {
            printf("%s\n", orden[i]);
            free(orden[i]);
        }
        printf("\n");
    }
    fclose(fp);
    return 0;
}
Community
  • 1
  • 1
chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • your answer did help a lot, because i realized that the loop was iterating one more time that expected –  Dec 10 '16 at 14:48
0

You can do the following:

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

static void play_with_token(char *token, char const *delim)
{
  if (token == NULL)
    return;
  printf(" %s", token);
  play_with_token(strtok(NULL, delim), delim);
}

int main(int argc, char *argv[])
{
    if (argc != 2)
      return 1;
    FILE *fp = fopen(argv[1], "r");
    if (fp == NULL)
      return 1;

    char *line = NULL;
    size_t len = 0;
    ssize_t read;
    while ((read = getline(&line, &len, fp)) != -1) {
      printf("parsing line :");
      char const *delim = " ";
      play_with_token(strtok(line, delim), delim);
      printf("\n");
    }
   free(line);
   fclose(fp);
}
Stargateur
  • 24,473
  • 8
  • 65
  • 91
  • 1
    Your tail recursion style in `play_with_token` is not idiomatic in C. It is especially bad style as the initial and subsequent calls to `strtok()` appear in different functions. `strtok()` semantics and non reentrancy issues are enough of an issue to make it highly advisable to keep all calls close together. – chqrlie Dec 10 '16 at 13:01
  • @chqrlie It's a style issue, I don't think that is bad because `play_with_token()` don't need to know the real string. I will just add delim to be more logic. – Stargateur Dec 10 '16 at 13:06
  • It may be a style issue, but how would you rate a purposely obfuscated answer? Making `play_with_token` local and `static` does not fix the poor pedagogical value of this approach. You are probably well aware of the shortcomings of `strtok()`, but most newbie reader aren't. – chqrlie Dec 10 '16 at 13:11
  • I think that it is not the purpose to display the actual OP. Process the file of the second item, for example. Your method is not suitable. – BLUEPIXY Dec 10 '16 at 13:13
  • @BLUEPIXY "I want to read it line by line and split it up with this delimiter " "", I just answer literally what he asks. – Stargateur Dec 10 '16 at 13:17
  • Yes, but it is scarce in actual use. – BLUEPIXY Dec 10 '16 at 13:20
  • @BLUEPIXY My answer can be used by everyone, and I answer the OP. A lot of people; maybe don't care about what the OP wants to do with that. The OP didn't ask "... and how to stock it in?". – Stargateur Dec 10 '16 at 13:26
  • Dare to say, you reorganized without consent. That is what He/She did not need to ask. – BLUEPIXY Dec 10 '16 at 13:31
  • 2
    @Stargateur: the tail recursion is not so much of an issue as the fact that you call `strtok(NULL, " ")` in `play_with_token()` assuming that `strtok(line, " ")` was called by the caller. This is what I find misleading for newbies. If you were to use the safer `strtok_r()` version, your approach would become more cumbersome. – chqrlie Dec 10 '16 at 13:33
0

For starters you should change the condition in the outer loop statement the following way

while ( fgets(ordenes, sizeof(ordenes), fp) != NULL )

The condition in the inner loop should be written at least like

  while ( tok < 3 && token != NULL) {

The tokens should be outputted in a loop and the allocated memory must be freed. For example

  for ( int i = 0; i < tok; i++ )
  {
      printf("\n%s\n", orden[i]);
      free( orden[i]);
  }
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
-1

try this:

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

int main(int argc, char *argv[]){
    char ordenes[150];
    char *orden[3];
    char *token;
    int tok;
    FILE *fp;

    fp = fopen (argv[1], "r");
    if(fp==NULL){
        printf("File error");
        exit(1);
    }
    while(fgets(ordenes, sizeof(ordenes), fp)){
        printf("%s\n",ordenes);
        token = strtok(ordenes, " ");
        tok = 0;
        while (token != NULL){
            orden[tok++] = strdup(token);
            token = strtok(NULL," ");
        }
        printf("\n%s\n",orden[0]);
        printf("\n%s\n",orden[1]);
        printf("\n%s\n",orden[2]);
        free(orden[0]);free(orden[1]);free(orden[2]);
    }
    fclose(fp);
}
BLUEPIXY
  • 39,699
  • 7
  • 33
  • 70
  • You have undefined behavior, `tok++` what if it's go over 2? `argv[1]` what if it's `NULL`? You didn't set orden elem to `NULL`, so `free` or use them is UB. – Stargateur Dec 10 '16 at 12:48
  • 1
    Assuming correct input format leads to undefined behavior and exploitable flaws. – chqrlie Dec 10 '16 at 12:50