Heap corruption while freeing memory in a recursion function

Question

I'm implementing an algorithm to select Kth smallest element of an array . so far when i was trying to free heap memory i got this error : crt detected that the application wrote to memory after end of heap buffer ...

int SEQUENTIAL_SELECT(int *S , int k , int n)
{
    if(n<=Q) // sort S and return the kth element directly
    {
        qsort(S,n,sizeof(int),compare);
        return S[k];
    }
    // subdivide S into n/Q subsequences of Q elements each
    int countSets = ceil((float)n/(float)Q);

    //sort each subsequnce and determine its median
    int *medians = new int[countSets];
    for(int i=0;i<countSets;i++)
    {
        if(i==countSets-1) 
        {
            int size = Q - (n%Q);
            qsort(&S[Q*i],size,sizeof(int),compare);
            medians[i] = S[i*Q+size/2];
            continue;
        }
        qsort(&S[Q*i],Q,sizeof(int),compare);
        medians[i] = S[i*Q+Q/2]; 
    }

    // call SEQUENTIAL_SELECT recursively to find median of medians 
    int m = SEQUENTIAL_SELECT(medians,countSets/2,countSets);
    delete[] medians;
    int size = (3*n)/4;

    int* s1 = new int[size]; // contains values less than m
    int* s3 = new int[size]; // contains values graten than m

    for(int i=0;i<size;i++)
    {
        s1[i] = INT_MAX;
        s3[i] = INT_MAX;
    }
    int i1=0;
    int i2=0;
    int i3=0;
    for(int i=0;i<n;i++)
    {
        if(S[i]>m)
            s3[i3++] = S[i];
        else if(S[i]<m)
            s1[i1++] = S[i];
        else
            i2++; // count number of values equal to m
    }

    if( i1>=k )
        m = SEQUENTIAL_SELECT(s1,k,i1);
    else if( i1+i2+i3 >= k)
        m = SEQUENTIAL_SELECT(s3,k-i1-i2,i3);


    delete[] s3;
    delete[] s1;

    return m;
}

Answer 1

@Dcoder is certainly correct that Q - n%q is incorrect. It should be n%Q. In addition, the computation size = (3*n)/4 is not reliable; try it with n = 6 (assuming, as seems certain, that Q is actually 5) given the vector [1, 2, 3, 4, 5, 0].

You could have avoided having a lot of eyes looking at your code by simply checking the values of the indexes at every array subscript assignment (although that wouldn't have caught the assignments inside of qsort, but more on that below).

It must surely have occurred to you that you are using an awful lot of memory to perform a simple operation, which could in fact be done in-place. Normally the reason to avoid doing an in-place operation would be that you need to preserve the original vector, but you're computing medians with qsort which sorts in-place, so the original vector is already modified. If that's acceptable, then there is no reason not to do the rest of the median-of-medians algorithm in-place. [1]

By the way, although I'm certainly not one of those who fears floating-point computations, there is no reason at all for countSets = ceil(float(n)/float(Q)). (n + Q - 1)/Q will work just fine. That idiom could usefully have been used in the computation of size as well, although I'm not at all sure where you got the 3n/4 computation from in the first place.

---

[Note 1] Hint: instead of grouping consecutively, divide the vector into five regions and find the median of the i^th element of each region. Once you've found it, swap it with the i^th element of the first region; once that is done, your first region -- the first fifth of the vector -- contains the medians and you can recurse on that subvector. That means actually writing out the median code as a series of comparisons, which is tedious but a lot faster than calling qsort. That also avoids the degenerate case I mentioned above, where the median-of-medians computation incorrectly returns the smallest element in the vector.