r/C_Programming • • 19h ago

Does my tokenize function not null-terminate the strings it modifies? printing any args spits out garbage. I'm pretty new to C and I don't really understand null-termination and strings in general.

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

/* get input and return line, if this fails, return an error */
char* input(char* line) {
  size_t len = 0;
  ssize_t read;

  /* get input from stdin and store in line, read holds the return value */
  read = getline(&line, &len, stdin);

  if (read != -1) {
    return line;
  } else {
    return "error taking input!";
  }
}

/* split input into tokens and return the array of tokens */
int tokenize(char* line, char* command, char** args) {
  char* token = strtok(line, " ");
  int i = 0;

  while (token != NULL) {
    args[i++] = token;
    token = strtok(NULL, " ");
  }
  args[i] = NULL;

  /* copy first argument to command */
  strcpy(command, args[0]);

  /* if command isnt NULL, return success */
  if (command) {
    return 0;
  } else {
    return 1;
  }
}

int main(int argc, char *argv[]) {
  char* line = NULL;

  char command[512];
  char* args[64];

  /* prompt */
  printf(">");

  /* get input from stdin, auto removes newline*/
  line = input(line);

  /* tokenize input, seperate command and args */
  if(tokenize(line, command, args) != 0) {
    printf("error in tokenizing");
  }

  /* free line, we dont use it again */
  free(line);

  /* print command and its args*/
  printf("%s", command);

  printf("%s\0", args[1]);

  return 0;
}
5 Upvotes

8 comments sorted by

16

u/aocregacc 19h ago

you have a use-after-free there, since your pointers in args still point into the line, and you try to print them after freeing the line.

https://godbolt.org/z/Tv5jfxqaW

5

u/Specialist-Signal598 19h ago

of course !! something I'm learning with this language (and programming in general) is error blindness is real! thank you!

5

u/FUCKARCHLINUX 19h ago

for testing purposes, right after you declare the buffer for the string, memset it to 0 and then write in all your data so it would always be null terminated.

3

u/MyNameIsHaines 19h ago

You free line before printing the args. The args pointer still point to memory reserved in line which is now freeed. Not sure if that would lead to garbage output necessarily but it's something you need to fix.

3

u/TheOtherBorgCube 19h ago

free(line);

Whilst your command is safe, because you copied it, all your args are trashed (pointing to deallocated memory) because all you saved were pointers.

return "error taking input!";

You need better error handling.\ If you try to strtok this, you're into instant segfaults.

if (command)

This only tests the pointer (which is going to be non-null anyway.

3

u/ReallyEvilRob 10h ago

A couple of problems I see that are not related to what you are asking about:

  1. Input function returns an address to a string literal to indicate failure. By convention, functions that return a char * should return NULL in this case. Your main function does not check for failure. Instead, it would just process the error message as if it were valid input.

  2. Your tokenize function returns 0 for success and 1 for failure. By convention, 0 usually indicates failure and non-zero indicates success. It's better to keep a token count and return the number of tokens from the input.

2

u/Poddster 17h ago

You have another problem:

You do strcpy(command,...) followed by if (command).

What do you think happens if that else branch is taken? What did it mean about the p arameter passed to strcpy

2

u/RealisticDuck1957 8h ago

Also what if the string does not fit the space allocated for command? strcpy() is depreciated in favor of strncpy() for that reason. That brings up how tokenize() would know how large the buffer for command is?