Output int from string input

Question

This takes a string input like 1,2,3,4,5, splits it using , delimiter, converts each char to int and output it in a single line like this : 12345

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
int* createIntArray(char* numbersInString, int* array_size);
int* createIntArray(char* numbersInString, int* array_size)
{
 char* token = strtok(numbersInString,",");
 int* numbersArray = malloc(sizeof(int)*100);
 int i = 0;
 while(token != NULL)
 {
 numbersArray[i] = (int)*token -'0';
 token = strtok(NULL,",");
 i++;
 }
 *array_size = i;
 return numbersArray;
}
int main(){
 char* stringNumbers = malloc(sizeof(char)*100);
 int* intArray;
 int arrayLength = 0;
 printf("Enter numbers\n");
 scanf("%s",stringNumbers);
 intArray = createIntArray(stringNumbers,&arrayLength);
 int i;
 for(i = 0; i < arrayLength;i++)
 {
 printf("%d",intArray[i]);
 }
 free(stringNumbers);
 free(intArray);
 return 0;
}

I know the code is super useless, but I'm starting to learn C and I wanted to see if my code was alright and if it complies to C coding standards.

• syb0rg: I spy an integer overflow exploit!
• IEatBagels: This would happen if I was happening to enter : 12379573853625767531596,1. Is that it?
• syb0rg: No, you would have to enter enough characters to fill up all of the buffers on the local stack, ~106-116 bytes/characters depending on the computer architecture.
• IEatBagels: Oh, not understanding what you mean means I should read your link :P
• syb0rg: This is a more complicated topic for a beginner to understand. Don't worry too much if you don't get the concept. If you were to use this same code for a login application though, then I would have to write a full-fledged answer on the exploitation process and prevention of the overflow.

Answer 1

A few things to consider, since this is a learning exercise. They don't really matter for your program, but maybe you'll avoid learning a few bad habits...

• while(token != NULL) would commonly be written while(token) instead.
• Learn about const. Use it whenever you can. It can make the meaning of the code clearer, and it can help the compiler find mistakes. Your createIntArray function could have parameters char* const numbersInString and int* const array_size, for example.
• Try to use a consistent style for naming things. numbersInString is one style. array_size is a different one. Either style works ok. Mixing them adds a bit of confusion.
• strtok is an anti-favorite of mine. It modifies the input string. Sometimes that's fine, but you could do this task without modifying the input. A function that works on a constant string will be useful in more places than a function that alters the input to get the same result.

• IEatBagels: What could I use instead of strtok? When I looked on Google it is the one that seemed the most suited for my case
• syb0rg: @TopinFrassi Use strtok_r.
• GraniteRobert: strtok_r also modifies the input string. I don't have a suggested one-size-fits-all replacement to suggest; it depends on the situation. In this case, I might start with sscanf to find one integer at a time from the string.
• IEatBagels: This was too easy ahah
• syb0rg: @GraniteRobert I know, but that is a standard thread-safe option for strtok. Using sscanf to find one integer at a time would be inefficient.

Answer 2

• Since you have main() below the other function, you don't need the prototype for that function. It's only needed if main() is defined before a function that it calls.

• There's no need to declare a variable and then assign to it:

  int* intArray;
  // ...
  intArray = createIntArray(stringNumbers,&arrayLength);
  

  Variables should just be initialized in the closest scope possible. This will ease maintenance as you won't have to search for the declared variable elsewhere, such as if it's no longer in use.

  int* intArray = createIntArray(stringNumbers,&arrayLength);
  

• Instead of incrementing i in createIntArray(), why not just increment array_size?

  Plus, i is more commonly used as a for loop counter, which isn't the case here. If someone were to take a glance at that while loop without further context, they may think that it should be a for loop instead. By making this change, you'll remove that possible misconception and better communicate your exact intentions.

• IEatBagels: I'm so focused on figuring pointers out that I forget the basics of programming!! Though I didn't know about your first point, thanks!
• Elias Van Ootegem: @TopinFrassi: Forgive my shameless self-promoting, but if you're trying to figure out pointers this might be of some use. People seemed to like my explanation there :)

Answer 3

There are a couple minor things I'd like to add to this: What if the string that is passed to createIntArray contains more than 100 ints? Currently, you're allocating 100 ints, regardless of how many you'll actually need. I'd choose to use a stack array as buffer, and realloc the dynamic memory in chunks. Then, I'd copy the buffer to this new memory (using memcpy).
I'd also change the type of array_size to size_t: it's unsigned, as a size should be, and it's more descriptive.

Before I start listing every single change I'd make, here's a re-written version of your function:

int* createIntArray(char* numbersInString, size_t* array_size)
{//string is not altered, use const
 char* token = strtok(numbersInString,",");
 //buffer array
 int buffer[100];
 int* numbersArray = NULL;//null ptr here
 int i = 0;
 //check if array_size is valid
 if (array_size == NULL)
 return NULL;//null-pointer!
 //initialize array_size to 0
 *array_size = 0;
 while(token)
 {
 buffer[i] = *token -'0';//no need to cast
 //or, if you #include <stdlib.h>
 buffer[i] = atoi(token);
 if (++i == 100)
 {//buffer is full
 *array_size += i;
 numbersArray = realloc(
 numbersArray,
 *array_size * sizeof *numbersArray
 );
 if (numbersArray == NULL)
 exit( EXIT_FAILURE );
 //copy buffer
 memcpy(
 numbersArray + (*array_size - i),
 buffer,
 sizeof buffer
 );
 i = 0;
 }
 }
 if (i)
 {//add remainder of buffer
 *array_size += i;
 numbersArray = realloc(
 numbersArray,
 *array_size * sizeof *numbersArray
 );
 if (numbersArray == NULL)
 exit( EXIT_FAILURE );
 memcpy(
 numbersArray + (*array_size - i),
 buffer,
 i * sizeof *buffer
 );
 }
 return numbersArray;
}

Ok, why did I change/add certain things:

• *array_size = 0;: The inital value of array_size should be 0, if you look at how/where I call realloc, it should be obvious why I did this.
• *array_size += i;: When the buffer is full, ++i will be 100, and the memory allocated by numbersArray has do be adjusted, to hold i additional values, each of sizeof *numbersArray.
• memcpy(numbersArray + (*array_size - i),arr, sizeof arr): This is the tricky bit (perhaps). The numbersArray could already hold 200 or 300 ints. Calling memcpy requires us to pass a (VALID!) pointer to the first block of memory that is actually available to write to. This is the array_size minus the size of the buffer (i). The source to copy from is, of course, the buffer array, and the amount of bytes to copy is, naturally, sizeof buffer.

When the while loop breaks, It's a simple matter of checking i's value, to see if there are new values in the buffer, and adding those to the dynamic memory array.
The memcpy call here is a little bit different. If you pass sizeof buffer here, you will invoke undefined behaviour (due to heap corruption). Instead, because we've re-allocated numbersArray to hold just i ints more, you have to pass i * sizeof *buffer instead. Read this expression as i times the size of whatever type buffer holds. IE, if i is 10, memcpy will copy the number of bytes, required to store 10 ints. No more.

Whether you choose to use this code or not, a couple of tips I would urge you to take to heart:

• avoid ptr = malloc(sizeof(type)*n), the safer, more common way to allocate memory is to use ptr = malloc(sizeof *ptr *n);. If the type of ptr is changed, you don't have to worry about the malloc's, sizeof *ptr is like saying "size of whatever type ptr points to".
• Always check pointers, be it those that are returned by malloc, calloc or realloc, or pointers that are passed to a function.
• perhaps unrelated: consider re-writing this function to return the array size (which allows you to return negatives to indicate errors), and instead take a int **target_array argument, along with a size_t current_size. so you can re-work this function to add values to an existing dynamic array, too.