using qsort causing a segmentation fault

Question

Well, as part of learning C++, my project has a restriction on it. I'm not allowed to use any libraries except the basic ones such as <cstring> and a few other necessities.

The project should take in input from a file that is an "n" number of columns of strings and be able to sort the output according to lexicographical ordering of any selected column. So for example, given the input

Cartwright   Wendy    93
Williamson   Mark     81
Thompson     Mark     100
Anderson     John     76
Turner       Dennis   56

It should sort them by column. And my search around StackOverflow returned a result from someone else who had to do the exact same project a few years ago too Hahaha Qsort based on a column in a c-string?

But in my case I just use a global variable for the column and get on with life. My problem came in when I am implementing the compare function for qsort

In my main method I call

qsort (data, i, sizeof(char*), compare);

where data is a char * data[] and i is the number of lines to compare. (5 in this case)

Below is my code for the compare method

int compare (const void * a, const void * b){
    char* line1 = new char[1000]; char* line2 = new char[1000];
    strcpy(line1, *((const char**) a));
    strcpy(line2, *((const char**) b));
    char* left = &(strtok(line1, " \t"))[column-1];
    char* right = &(strtok(line2, " \t"))[column-1];
    return strcmp(left, right);
}

the 1000s are because I just generalized (and did bad coding on purpose) to overgeneralize that no lines will be longer than 1000 characters.

What confuses me is that when I use the debugger in Eclipse, I can see that it it compares it successfully the first time, then on the second round, it has a segmentation fault when it tries to compare them.

I also tried to change the code for assigning left and right to what is below but that didn't help either

char* left = new char[100];
strcpy(left, &(strtok(line1, " \t"))[column-1]);
char* right = new char[100];
strcpy(right, &(strtok(line2, " \t"))[column-1]);

Please help me understand what is causing this segmentation fault. The first time it compares the two, left = "Williamson" and right = "Thompson". The second time it compares (and crashes trying) left = "Cartwright" and right = "Thompson"

Answer 1

char* line1 = new char[1000]; char* line2 = new char[1000];

This is not good at all. You're never freeing this, so you leak 2000 bytes every time your comparison function is called. Eventually this will lead to low-memory conditions and new will throw. (Or on Linux your process might get killed by the OOM-killer). It's also not very efficient when you could just have said char line1[1000], which is super-quick because it simply subtracts from the stack pointer rather than potentially traversing a free list or asking the kernel for more memory.

But really you could be doing the compare without modifying or copying the strings. For example:

static int
is_end_of_token(char ch)
{
    // If the string has the terminating NUL character we consider it the end.
    // If it has the ' ' or '\t' character we also consider it the end.  This
    // accomplishes the same thing as your strtok call, but WITHOUT modifying
    // the source buffer.
    return (!ch || ch == ' ' || ch == '\t');
}

int
compare(const void *a, const void *b)
{
   const char *strA = *(const char**)a;
   const char *strB = *(const char**)b;

   // Loop while there is data left to compare...
   while (!is_end_of_token(*strA) && !is_end_of_token(*strB))
   {
      if (*strA < *strB)
         return -1;      // String on left is smaller
      else if (*strA > *strB)
         return 1;       // String on right is smaller
      ++strA;
      ++strB;
   }

   if (is_end_of_token(*strA) && is_end_of_token(*strB))
      return 0;    // both strings are finished, so they are equal.
   else if (is_end_of_token(*strA))
      return -1;   // left string has ended, but right string still has chars
   else
      return 1;    // right string has ended, but left string still has chars
}

But lastly... You're using std::string you say? Well, if that's the case, then assuming the memory passed to qsort is compatible with "const char **" is a little weird, and I would expect it to crash. In that sense maybe what you should do something like:

int compare(const void *a, const void *b)
{
    const char *strA = ((const std::string*)a)->c_str();
    const char *strB = ((const std::string*)b)->c_str();

    // ...
}

But really, if you are using C++ and not C, you should use std::sort.