Skip to main content
deleted 96 characters in body
Source Link
JS1
  • 28.6k
  • 3
  • 41
  • 83

###Input format###

There'sThere are a few things I don't like about the way input is handled:

  1. Having all of your input done via the command line is awkward. Suppose you wanted to try a test case of 10 million integers. How would you even do that? I propose that you read your input from stdin instead. That way you could just create a test case input file and run your program like countsort < input.txt to test it.
  2. I don't feel like the user should have to specify the maximum value as the second input value. Your program could just compute the maximum value as it reads in all the numbers. After all, your program doesn't need to use the max value until it is ready to do the sort. This would also allow you to remove some of the error checking you have where you test the array values against the maximum value.
  3. I don't think there is a need for the [ and ] parts of the input, and also the comma separators. It would be simpler to just have the input be: the count n followed by n numbers without any extra brackets or commas.

###Overflow in size computation###

It looks like you took care of the special case where the maximum value was UINT_MAX, because in your call to malloc() you used the value max+1 and didn't want that to overflow to zero. However, there are many other ways to overflow in that same line:

unsigned int *counts = malloc(sizeof(int)*(maxval+1));

Suppose you were on a 32-bit platform with size_t being 32-bits wide, and suppose maxval were 0x40000000. The expression sizeof(int) * (maxval+1) would evaluate to 4 * 0x40000001 or just 4 because the result would be truncated to 32 bits. Note that 0x40000000 is just one example. Any value greater than or equal to 0x3fffffff would cause problems for this platform.

###Bug: using uninitialized memory###

You allocate your counts array using malloc(), but you never clear your counts array to 0 before you start to use it. So you could end up with garbage values for your counts. I would suggest using calloc() so that your allocated array is cleared to zero automatically. A side benefit to using calloc() is that it avoids your overflow problem because the arguments are split into a size and a count:

unsigned int *counts = calloc(maxval+1, sizeof(int));

###Overflow in counts###

If you were on a 64-bit platform with 64-bit size_t and 32-bit unsigned int, you could run into another type of overflow problem. Since your counts array is of type unsigned int (32-bit), your counts could overflow because your array could have more than 2^32 elements. For example, suppose your array had 5 billion instances of the number 5. Your program would overflow counts[5] and get the wrong count in the end.

###Input format###

There's a few things I don't like about the way input is handled:

  1. Having all of your input done via the command line is awkward. Suppose you wanted to try a test case of 10 million integers. How would you even do that? I propose that you read your input from stdin instead. That way you could just create a test case input file and run your program like countsort < input.txt to test it.
  2. I don't feel like the user should have to specify the maximum value as the second input value. Your program could just compute the maximum value as it reads in all the numbers. After all, your program doesn't need to use the max value until it is ready to do the sort. This would also allow you to remove some of the error checking you have where you test the array values against the maximum value.
  3. I don't think there is a need for the [ and ] parts of the input, and also the comma separators. It would be simpler to just have the input be: the count n followed by n numbers without any extra brackets or commas.

###Overflow in size computation###

It looks like you took care of the special case where the maximum value was UINT_MAX, because in your call to malloc() you used the value max+1 and didn't want that to overflow to zero. However, there are many other ways to overflow in that same line:

unsigned int *counts = malloc(sizeof(int)*(maxval+1));

Suppose you were on a 32-bit platform with size_t being 32-bits wide, and suppose maxval were 0x40000000. The expression sizeof(int) * (maxval+1) would evaluate to 4 * 0x40000001 or just 4 because the result would be truncated to 32 bits. Note that 0x40000000 is just one example. Any value greater than or equal to 0x3fffffff would cause problems for this platform.

###Bug: using uninitialized memory###

You allocate your counts array using malloc(), but you never clear your counts array to 0 before you start to use it. So you could end up with garbage values for your counts. I would suggest using calloc() so that your allocated array is cleared to zero automatically. A side benefit to using calloc() is that it avoids your overflow problem because the arguments are split into a size and a count:

unsigned int *counts = calloc(maxval+1, sizeof(int));

###Overflow in counts###

If you were on a 64-bit platform with 64-bit size_t and 32-bit unsigned int, you could run into another type of overflow problem. Since your counts array is of type unsigned int (32-bit), your counts could overflow because your array could have more than 2^32 elements. For example, suppose your array had 5 billion instances of the number 5. Your program would overflow counts[5] and get the wrong count in the end.

###Input format###

There are a few things I don't like about the way input is handled:

  1. Having all of your input done via the command line is awkward. Suppose you wanted to try a test case of 10 million integers. How would you even do that? I propose that you read your input from stdin instead. That way you could just create a test input file and run your program like countsort < input.txt to test it.
  2. I don't feel like the user should have to specify the maximum value as the second input value. Your program could just compute the maximum value as it reads in all the numbers. This would also allow you to remove some of the error checking you have where you test the array values against the maximum value.
  3. I don't think there is a need for the [ and ] parts of the input, and also the comma separators. It would be simpler to just have the input be: the count n followed by n numbers without any extra brackets or commas.

###Overflow in size computation###

It looks like you took care of the special case where the maximum value was UINT_MAX, because in your call to malloc() you used the value max+1 and didn't want that to overflow to zero. However, there are many other ways to overflow in that same line:

unsigned int *counts = malloc(sizeof(int)*(maxval+1));

Suppose you were on a 32-bit platform with size_t being 32-bits wide, and suppose maxval were 0x40000000. The expression sizeof(int) * (maxval+1) would evaluate to 4 * 0x40000001 or just 4 because the result would be truncated to 32 bits. Note that 0x40000000 is just one example. Any value greater than or equal to 0x3fffffff would cause problems for this platform.

###Bug: using uninitialized memory###

You allocate your counts array using malloc(), but you never clear your counts array to 0 before you start to use it. So you could end up with garbage values for your counts. I would suggest using calloc() so that your allocated array is cleared to zero automatically. A side benefit to using calloc() is that it avoids your overflow problem because the arguments are split into a size and a count:

unsigned int *counts = calloc(maxval+1, sizeof(int));

###Overflow in counts###

If you were on a 64-bit platform with 64-bit size_t and 32-bit unsigned int, you could run into another type of overflow problem. Since your counts array is of type unsigned int (32-bit), your counts could overflow because your array could have more than 2^32 elements. For example, suppose your array had 5 billion instances of the number 5. Your program would overflow counts[5] and get the wrong count in the end.

Source Link
JS1
  • 28.6k
  • 3
  • 41
  • 83

###Input format###

There's a few things I don't like about the way input is handled:

  1. Having all of your input done via the command line is awkward. Suppose you wanted to try a test case of 10 million integers. How would you even do that? I propose that you read your input from stdin instead. That way you could just create a test case input file and run your program like countsort < input.txt to test it.
  2. I don't feel like the user should have to specify the maximum value as the second input value. Your program could just compute the maximum value as it reads in all the numbers. After all, your program doesn't need to use the max value until it is ready to do the sort. This would also allow you to remove some of the error checking you have where you test the array values against the maximum value.
  3. I don't think there is a need for the [ and ] parts of the input, and also the comma separators. It would be simpler to just have the input be: the count n followed by n numbers without any extra brackets or commas.

###Overflow in size computation###

It looks like you took care of the special case where the maximum value was UINT_MAX, because in your call to malloc() you used the value max+1 and didn't want that to overflow to zero. However, there are many other ways to overflow in that same line:

unsigned int *counts = malloc(sizeof(int)*(maxval+1));

Suppose you were on a 32-bit platform with size_t being 32-bits wide, and suppose maxval were 0x40000000. The expression sizeof(int) * (maxval+1) would evaluate to 4 * 0x40000001 or just 4 because the result would be truncated to 32 bits. Note that 0x40000000 is just one example. Any value greater than or equal to 0x3fffffff would cause problems for this platform.

###Bug: using uninitialized memory###

You allocate your counts array using malloc(), but you never clear your counts array to 0 before you start to use it. So you could end up with garbage values for your counts. I would suggest using calloc() so that your allocated array is cleared to zero automatically. A side benefit to using calloc() is that it avoids your overflow problem because the arguments are split into a size and a count:

unsigned int *counts = calloc(maxval+1, sizeof(int));

###Overflow in counts###

If you were on a 64-bit platform with 64-bit size_t and 32-bit unsigned int, you could run into another type of overflow problem. Since your counts array is of type unsigned int (32-bit), your counts could overflow because your array could have more than 2^32 elements. For example, suppose your array had 5 billion instances of the number 5. Your program would overflow counts[5] and get the wrong count in the end.