2

I'm working in C

I have a struct called Entity and I create a dynamic array of that struct. Then I try to remove one element from the array but I don't get the behaviour I want.

Here is the code I'm using:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct Entity
{
    int x, y;
    int velX, velY;
}Entity;

int remove_element(Entity** array, int sizeOfArray, int indexToRemove)
{
    int i;

    printf("Beginning processing. Array is currently: ");
    for (i = 0; i < sizeOfArray; ++i)
        printf("%d ", (*array)[i].x);
    printf("\n");

    Entity* temp = malloc((sizeOfArray - 1) * sizeof(Entity)); // allocate an array with a size 1 less than the current one

    memmove(
            temp,
            *array,
            (indexToRemove+1)*sizeof(Entity)); // copy everything BEFORE the index

    memmove(
            temp+indexToRemove,
            (*array)+(indexToRemove+1),
            (sizeOfArray - indexToRemove)*sizeof(Entity)); // copy everything AFTER the index


    printf("Processing done. Array is currently: ");
    for (i = 0; i < sizeOfArray - 1; ++i)
        printf("%d ", (temp)[i].x);
    printf("\n");

    free (*array);
    *array = temp;
    return 0;

}

int main()
{
    int i;
    int howMany = 20;

    Entity* test = malloc(howMany * sizeof(Entity*));

    for (i = 0; i < howMany; ++i)
        (test[i].x) = i;

    remove_element(&test, howMany, 14);
    --howMany;

    return 0;
}

And the output I get :

Beginning processing. Array is currently: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
Processing done. Array is currently: 0 1 2 3 4 1866386284 6 7 8 9 10 11 12 13 15 16 17 18 19

Then the program crashes at the free (*array); line. I want my second line to be 0 1 2 3 4 5 6 7 8 9 10 11 12 13 15 16 17 18 19.

How could I solve my problem ?

Drakalex
  • 1,488
  • 3
  • 19
  • 39
  • Undefined behavior – user2736738 Jan 20 '18 at 13:51
  • At what point exactly is the content faulty? It's the line before that which causes the bug. BTW: The point of `memmove()` is that source and target can overlap, since you copy to new memory, you could use `memcpy()`. – Ulrich Eckhardt Jan 20 '18 at 13:56
  • Consider what happens if `indexToRemove` is zero. The first `memmove()` copies one data structure (when it should copy none) and the second copies `sizeOfArray` structures to a buffer/array that contains `sizeOfArray-1` such structures. The behaviour is therefore undefined, even in that simple case. In short: you need to check your bounds better. – Peter Jan 20 '18 at 13:57
  • @Peter I simply added `if(indexToRemove > 0)` before the first memmove(), and `if(indexToRemove < sizeOfArray - 1)` before the second one, that should do it for the bounds ? – Drakalex Jan 20 '18 at 14:04
  • @Drakalex.: I have added an edit. If you were using 0 indexing should follow these – user2736738 Jan 20 '18 at 14:07
  • @coderredoc Yes this is simplier your way than with two if, but it's weird because if I set the index to remove at 0, the program runs fine, but for every other value it crashes at the free call. – Drakalex Jan 20 '18 at 14:10
  • @Drakalex - those tweaks help, but still don't address the problem of undefined behaviour (copying more to a buffer than it can hold) I mentioned previously. – Peter Jan 20 '18 at 14:32
  • @Peter.: Well with the changes made it would work (By work I mean no error in copying data) ...what do you say or suggest? Do you notice the OP is deleting one element? – user2736738 Jan 20 '18 at 14:33
  • @coderredoc - do you notice that the amount of data being copied to `temp` exceeds the amount of memory allocated? – Peter Jan 20 '18 at 14:47
  • @Peter.: Correct me if I am wrong - OP is removing one element and so OP allocates one less than there was before this. So OP allocates `sizeofarray - 1` elements to copy `sizeofarray-1` elements.. where is it wrong? – user2736738 Jan 20 '18 at 14:48
  • @coderredoc - You're missing an "off by one" error in the second `memmove()` call. It copies `sizeOfArray - indexToRemove` objects starting at `temp+indexToRemove`. This means it writes to `temp[sizeOfArray-1]` when temp only has `sizeOfArray - 1` elements allocated. The maximum valid index for `temp` is `sizeOfArray-2`. – Peter Jan 20 '18 at 14:52
  • @Peter.: I have already answeerd that ...you can check my answer. That was indeed wrong in OP's original posted code..I have mentioned it in my answer. You said that it is *still don't address the problem*..that's why I asked. – user2736738 Jan 20 '18 at 14:53

4 Answers4

2

First thing you have allocated memory space for holding 20 Enity*. Then you have dereferenced it (and the value it contained is indeterminate). This is undefined behavior. And all story ends here.

But let's analyze what you mostly wanted.

Entity* test = malloc(howMany * sizeof(Entity));
                                       ^^^^^^^

is what you wanted. Because only if you do this you will get the member elements x and so on.

Also if you are considering 0 indexing then the memmove calls should be

memmove(temp, *array, (indexToRemove)*sizeof(Entity)); 
memmove(temp+indexToRemove, (*array)+(indexToRemove+1), 
            (sizeOfArray - indexToRemove - 1)*sizeof(Entity)); 

These two changes will be enough to solve the problems you are facing and realizing the correct behavior. (If this is all there is in your code).

Also as per standard the main() should be declared like this in case it doesn't take any parameter int main(void). Free the dynamically allocated memory when you are done working with it. Also you should check the return value of malloc - in case it fails it returns NULL and you should handle that case.

user2736738
  • 30,591
  • 5
  • 42
  • 56
  • This solved the first problem, my new array is what I wanted, but it still crashes at the `free (*array);` line – Drakalex Jan 20 '18 at 13:59
  • In your second memmove, shouldn't it be `temp+indexToRemove*sizeof(Entity)` (and similar with the second argument)? – Stephan Lechner Jan 20 '18 at 14:14
  • @StephanLechner.: No it's not needed....pointer arithmetic is dictated by what it points to ...so it will move that much block itself. – user2736738 Jan 20 '18 at 14:21
  • @coderredoc Thanks you it finally work ! What did you change ? – Drakalex Jan 20 '18 at 14:25
  • @Drakalex.: Your earlier code had off by one error in the second `memmove` also. That's what I corrected. `(sizeOfArray - indexToRemove )` --> `(sizeOfArray - indexToRemove - 1)` – user2736738 Jan 20 '18 at 14:26
  • @Drakalex.: Also try to incorpoarte the changes that I mentioned in last paragraph. – user2736738 Jan 20 '18 at 14:27
0

Your offset calculations are off by one in both memmove instances. Use this instead:

// copy everything BEFORE the index
memmove(temp,
        *array,
        indexToRemove * sizeof(Entity));

// copy everything AFTER the index
memmove(temp + indexToRemove,
        *array + indexToRemove + 1,
        (sizeOfArray - indexToRemove - 1) * sizeof(Entity));
chqrlie
  • 131,814
  • 10
  • 121
  • 189
0

In main itself your memeory allocation is not done properly.if you are using double pointer you should allocate memory first for double pointer and than single pointer in loop one by one.

Rohit
  • 142
  • 8
  • Not a double pointer (`Entity* test = ...`) then the *address of* `test` is passed to `remove_element`. – David C. Rankin Jan 22 '18 at 03:51
  • Entity* test = malloc(howMany * sizeof(Entity*)); for (i = 0; i < howMany; ++i) (test[i].x) = i;//acessing without allocating memory. run with valgrind invalid read/write will come – Rohit Jan 22 '18 at 03:54
  • Exactly... How many `'*'` do you see between `Entity` and `test`? – David C. Rankin Jan 22 '18 at 03:55
  • You are allocating memory by multiplying size of *Entity and size of entity always be 4 byte since it is address .Not able to uderstand whether idea to allocate memory for structure element or structure pointer.Currently this code is invalid. – Rohit Jan 22 '18 at 07:07
0

a little touch

remove element in any type of struct array

regards

int remove_element(void **input_ptr, int input_size, int index_remove, int struct_size)
{
    void *temp_ptr;

    temp_ptr = malloc((input_size - 1) * struct_size);
    if (temp_ptr == 0)
        return -1;

    memmove(temp_ptr, *input_ptr, index_remove * struct_size);

    memmove(temp_ptr + (index_remove * struct_size), (*input_ptr) + (index_remove + 1) * struct_size, (input_size - index_remove - 1) * struct_size);

    free(*input_ptr);

    *input_ptr = temp_ptr;

    return 1;
}

usage example for question struct

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct Entity
{
    int x, y;
    int velX, velY;
}Entity;

int remove_element(void **input_ptr, int input_size, int index_remove, int struct_size)
{
    void *temp_ptr;

    temp_ptr = malloc((input_size - 1) * struct_size);
    if (temp_ptr == 0)
        return -1;

    memmove(temp_ptr, *input_ptr, index_remove * struct_size);

    memmove(temp_ptr + (index_remove * struct_size), (*input_ptr) + (index_remove + 1) * struct_size, (input_size - index_remove - 1) * struct_size);

    free(*input_ptr);

    *input_ptr = temp_ptr;

    return 1;
}

int main()
{
    int i;
    int howMany = 20;

    Entity* test = malloc(howMany * sizeof(Entity));

    for (i = 0; i < howMany; ++i)
    {
        (test[i].x) = i;
        printf("test[%d].x = '%d'\n", i, test[i].x);
    }

    remove_element((void**)&test, howMany, 14, sizeof(Entity));
    --howMany;

    printf("Deleted index --- new array\n");
    for (i = 0; i < howMany; ++i)
        printf("test[%d].x = '%d'\n", i, test[i].x);

    return 0;
}