Reading tokenized numbers from a file

Question

int *getUserInput(FILE *fpin, int *elementCount) {
  int *userInput;
  // Load file into buffer
  int length = fileLength(fpin);
  char *buffer = (char *) malloc(length * sizeof(char));
  int written = fread(buffer, sizeof(char), length, fpin);
  // Number of int's allocated in userInput
  int allocated = 10;
  *elementCount = 0;
  // Allocate space for the numbers
  userInput = (int *) malloc(allocated * sizeof(int));
  // "tokenize" the buffer with " " and "\n" as delimiters
  char *tokenized = strtok(buffer, " \n");
  while (tokenized != NULL) {
   if (allocated == *elementCount) {
      allocated *= 2;
      // Reallocate space based on need
      realloc(userInput, allocated * sizeof(int));
    }
    // Store the number in the array
    userInput[(*elementCount)++] = atoi(tokenized);
    tokenized = strtok(NULL, " \n");
  }
  realloc(userInput, *elementCount * sizeof(int));
  free(buffer);
  return userInput;
}

It's a function I wrote for reading in numbers from a file, delimited by a space. Curious to hear any thoughts/possible improvements.

Answer 1

Check that malloc didn't return NULL; in general, use defensive programming; That means always assume things went wrong, and check all return values. It helps with debugging too.

You should be aware that strtok and atoi are not safe functions for arbitrary user input. That means you are vulnerable to stack/buffer overflows. Read the man pages of the two functions carefully. Consider using \0-safe functions instead, or at least put a \0 at the end of the input strings.

Answer 2

int *getUserInput(FILE *fpin, int *elementCount) {
  int *userInput;
  // Load file into buffer
  int length = fileLength(fpin);
  char *buffer = (char *) malloc(length * sizeof(char));

You may want to check the return value of malloc. It could return NULL. However, many people don't because its rare and there's not a lot you can do to recover when it happens.

  int written = fread(buffer, sizeof(char), length, fpin);

Why is your variable called written when you are reading? Why are you storing this in a variable and never do anything with it? You should really check the return value because it may indicate there was a problem reading the file. If you just ignore it, that'll be really confusing if it happens. Fread doesn't put the null at the end of what it reads, you need to do that manually if you are going to use string functions.

  // Number of int's allocated in userInput
  int allocated = 10;
  *elementCount = 0;
  // Allocate space for the numbers
  userInput = (int *) malloc(allocated * sizeof(int));

In actual C, you don't need to case the return value of malloc.

  // "tokenize" the buffer with " " and "\n" as delimiters
  char *tokenized = strtok(buffer, " \n");
  while (tokenized != NULL) {
   if (allocated == *elementCount) {
      allocated *= 2;
      // Reallocate space based on need
      realloc(userInput, allocated * sizeof(int));

realloc is allowed to return a new pointer, having moved your data elsewhere. But you don't look at the return value which means your code will fail.

    }

You code is a mix on input and keep track of the array of items. I suggest splitting out the logic dealing with the list into its own function.

    // Store the number in the array
    userInput[(*elementCount)++] = atoi(tokenized);
    tokenized = strtok(NULL, " \n");
  }
  realloc(userInput, *elementCount * sizeof(int));

Again, realloc may move your pointer. You need to look at the return value. Also consider what happens if there is an allocation error?

  free(buffer);
  return userInput;
}

Answer 3

Looking at the code

Lets make it readable by indenting more than one character! Two is acceptable but more usall is around four.

OK we assume this is reading from some file descriptor but it is not a standard function.

  int length = fileLength(fpin);

The worst problem with C code is that people don't check the error codes from system function calls. ALWAYS check the error code. There may not be mcuh you can do (apart from log a message). BUT it is nice to know that you crashed because you ran out of memory rather than there being a bug in your code. Since you are using functions that assume the buffer is correctly terminated I would add an extra character to the buffer and stick '\0' into the location.

  char *buffer = (char *) malloc((length+1) * sizeof(char));

  // Add
  if (buffer == NULL)
  {   printf(stdout, "malloc() failed\n";
      exit(1);
  }
  buffer[length] = '\0'

The fread() function may fail you need to check the amount read is what you expected (also you should probably re-name to make it easier to understand).

  int readObjects = fread(buffer, sizeof(char), length, fpin);

  // Add
  if (readObjects != length)
  {
      if (feof(fpin))    { /* Report unexpected EOF */ }
      if (ferror(fpin))  { /* Report ERROR */}
  }

The realloc() function is very often used incorrectly. It it returns NULL when it failed to re-alloc a bigger space (but then the old memory is not freed). So you must catch the result of realloc into a temporary and only swap with your buffer if non NULL. Since we don't know if the realloc will work don't update your allocated value until you know it has worked.

   if (allocated == *elementCount) {
      allocated *= 2;
      // Reallocate space based on need
      realloc(userInput, allocated * sizeof(int));
    }

    // Should be written like this:

    if (allocated == *elementCount)
    {
        int* tmp = realloc(userInput, allocated * sizeof(int) * 2);
        if (tmp == NULL)
        {
            printf("realloc() failed\n";
            exit(1);
        }
        allocated *= 2;
        userInput = tmp;
    }

According to the documentaion atoi() has been deprecated in favor of strtol(), and is equivalent to (int)strtol(str, (char **)NULL, 10) So you should probably use that. And another point to check for errors.

    userInput[(*elementCount)++] = atoi(tokenized);

Technique used.

I would not have bothered to read the data into its own buffer (then tokenize it). The C library has a perfectly good stream reading library in scanf(). The same functionality can be achieved using:

int value;
while(fscanf(fpin, " %d",&value) == 1)
{
    // We read a value now store it.
}

Note the format string: " %d"
The leading space ignores multiple (including zero) white space characters proceeding your data (Note: White space includes '\n' so it will correctly wrap past the end of line). Then the %d will read a number from the stream. If it succeeds then it will return 1 to indicate the number of decoded '%' objects is 1.

Answer 4

Please note some of the following comments might overlapp with previous posts.

• allocated and elementCount should be unsigned type. length should be checked for errors - assuming that's the reason for it to be signed.

• written should be size_t and is unused. It should also be checked in case file size changed.

• malloc return value should be checked. If allocation failed, buffer should neither be used nor freed.

• realloc does not necessarily reallocate in place. Returned pointer shoud be error checked and used if allocation succeeded

• if the file does not contain a terminating null character, strtok will process past the allocated buffer.

• if the file size is 0, the last realloc will free userInput and return null. getUserInput() would then return a freed pointer.

• exception handling is missing on file access

• potential integer overflows

  • allocated *= 2
  • realloc(userInput, allocated * sizeof(int));
  • userInput[(*elementCount)++] = atoi(tokenized);