0

I've been trying to do the following Kata on Codewars:

The objective is to return all pairs of integers from a given array of integers that have a difference of 2.

The result array should be sorted in ascending order of values.

Assume there are no duplicate integers in the array. The order of the integers in the input array should not matter.

This is my solution, but its crashing with SIGSEGV (11). Invalid memory access.

Can someone please help?

#include <stdlib.h>

typedef struct Integer_Pair {
    int a;
    int b;
} pair;

// N.B. assign the size of the return array to the pointer *z
size_t twos_difference(size_t n, const int array[n], size_t *z) {

    pair* tot = malloc(sizeof(pair));

    if (tot == NULL)
        return 0;

    int temp[n];

    int val;

    // Counter for tot
    int c = 0;

    // Copy array elements in temp for later comparison
    for (size_t i = 0; i < n; i++)
    {
        temp[i] = array[i];
    }


    // Sort array and store in ascending order into temp 
    for (size_t i = 0; i < n; i++)
    {
        for (size_t j = i + 1; j < n; j++)
        {
            if (temp[i] > temp[j])
            {
                val = temp[i];
                temp[i] = temp[j];
                temp[j] = val;
            }
        }
    }

    // If difference between 2 numbers is 2, store in tot
    for (size_t i = 0; i < n; i++)
    {
        for (size_t j = 0; j < n; j++)
        {
            if (temp[i] - array[j] == -2)
            {
                tot[c].a = array[i];
                tot[c].b = temp[j];
                c++;
            }
        }
    }

    // Copy number of pairs with difference of 2 in tot
    *z = c;

    return *z;

    free(tot);

}
WhozCraig
  • 65,258
  • 11
  • 75
  • 141
DM880
  • 58
  • 6
  • With a Segmentation Fault, the first question should always be when using arrays "Am I trying to access memory that I don't have access to?" As I can't see the declared sizes of your arrays, this would be my best advice to you. – Joe Davis Nov 12 '20 at 15:32
  • 2
    `for(size_t j = + 1; j < n; j++)` ? I think you meant something else here – Eugene Sh. Nov 12 '20 at 15:33
  • yess, i + 1, I correct it – DM880 Nov 12 '20 at 15:35
  • Are the sizes of temp[] and array[] the same? What are they exactly? – Joe Davis Nov 12 '20 at 15:36
  • ```pair* tot = malloc(sizeof(pair));``` Also, this should be casted ```pair* tot = (pair*) malloc(sizeof(pair));``` – Joe Davis Nov 12 '20 at 15:39
  • yes ,same size,(size_t n) – DM880 Nov 12 '20 at 15:40
  • 2
    @NullPointer This is C. casting to/from `void*` is [neither required nor recommended](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – WhozCraig Nov 12 '20 at 15:47
  • regarding; `return *z; free(tot);` the call to `free()` will never be executed because the function has already exited due to the prior call to `return` – user3629249 Nov 12 '20 at 23:44
  • OT: regarding: `int c = 0;` The variable `c` is being assigned to a `size_t`. To avoid possible errors, the statement should be: `size_t c = 0;` – user3629249 Nov 12 '20 at 23:49
  • Please post a [mcve] so we can reproduce the problem and help you debug it. – user3629249 Nov 12 '20 at 23:51
  • OT: regarding: `if (tot == NULL) return 0;` This fails to tell the user there is a problem nor what the problem is. Suggest: `if (tot == NULL) { perror( "malloc failed" ); return 0; }` Which will output to `stderr` both the error message and the text reason the system thinks the error occurred. – user3629249 Nov 12 '20 at 23:58
  • regarding: `for (size_t i = 0; i < n; i++) { for (size_t j = 0; j < n; j++) { if (temp[i] - array[j] == -2)` the array `temp[]` is already sorted, so no need to be looking at: `array[]` Rather, just look at `temp[]` similar to: `for (size_t i = 0; i < (n-1); i++) { if( (temp[i+1] - temp[i]) == 2 ) z[i].a = temp[i]; z[i].b = temp[i+1]; } }` – user3629249 Nov 13 '20 at 00:11
  • also, for robustness, initialize the `z[]` to all -1 before checking for 'difference by 2' Then the caller will be able to easily determine when all the 'valid' pairs have been examined. – user3629249 Nov 13 '20 at 00:16
  • given the posted code, each pair will be 'counted' twice. This could (if lots of pairs with difference of 2) overrun an array of `n` values in the `array[]` which would also result in undefined behavior and could result in a seg fault event – user3629249 Nov 13 '20 at 00:24

2 Answers2

4

pair* tot = malloc(sizeof(pair));

Here, you are allocating only one instance of pair.

tot[c].a = array[i];
tot[c].b = temp[j];

Here, you are accessing more than one instance of pair

You need allocate more elements. pair* tot = malloc(n*sizeof(pair));

Krishna Kanth Yenumula
  • 2,533
  • 2
  • 14
  • 26
  • 4
    @DM880 without a proper [mcve] that's probably the best you're gonna get here. That said, the code you posted has zero reason for using `tot` in the first place. It is allocated, data stored in it, but never referenced thereafter anywhere, and due to the premature return statement, ultimately leaked. – WhozCraig Nov 12 '20 at 16:02
  • 1
    @DM880 In the question, you are supposed to return the pairs , not the number of pairs. Please check the question. – Krishna Kanth Yenumula Nov 12 '20 at 16:03
  • @WhozCraig sorry for that ,I'm new to this,I'll try to follow the proper minimal reproducible example next time – DM880 Nov 12 '20 at 16:16
-1

pair* tot = malloc(sizeof(pair));
should be changed to
pair* tot = (pair *)malloc(sizeof(pair));
and your return statement
if (tot == NULL)
should have the statement on the same line or indented:
if (tot == NULL) return 0;
The first change will fix your SIGSEGV exception, the second is just good coding practice and makes your code more readable.

Joe Davis
  • 333
  • 1
  • 8
  • 4
    There is no need to cast the result of malloc. See this https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc#:~:text=If%20you%20use%20malloc%20in,will%20return%20a%20void*%20type. – Krishna Kanth Yenumula Nov 12 '20 at 15:49
  • I've tried but nothing changed,thanks for the advice – DM880 Nov 12 '20 at 15:50
  • 2
    The first change will have no effect whatever on the compiled code and so will certainly not fix the segfault. Nor will the second, of course. – Nate Eldredge Nov 12 '20 at 16:04