0

I have the following C function:

void mySwap(void * p1, void * p2, int elementSize)
{
    void * temp = (void*) malloc(elementSize);
    assert(temp != NULL);
    memcpy(temp, p1, elementSize);
    memcpy(p1, p2, elementSize);
    memcpy(p2, temp, elementSize);
    free(temp);
}

that I want to use in a generic sorting function. Let's suppose that I use it to sort a dynamically allocated array owned by main(). Now let's suppose that at some point temp in mySwap() is actually NULL and the whole program is aborted without freeing the dynamically allocated array in main(). I thought that both mySwap() and the sorting function could return a bool value indicating whether the allocation was successful or not and by using if statements I could free the array in main() and exit(EXIT_FAILURE), but it doesn't seem like a very elegant sollution. What would be a good way to prevent a memory leak in such an instance?

Franek
  • 11
  • 1
  • 3
  • 3
    If your program is aborted, how could you have a memory leak? – Boiethios Mar 01 '18 at 13:41
  • Affter the malloc, temp never changes (it is never assigned to), `*temp` can change, but it is never tested. – joop Mar 01 '18 at 13:42
  • @joop, please see the `memcpy`. The code swaps two elements of varying sizes. – Paul Ogilvie Mar 01 '18 at 13:43
  • 1
    As a side note, perhaps a better approach would be to share a state/context between calls to mySwap. If your sorting isn't multithreaded, it wouldn't have to allocate on each call (i.e. `mySwap(void * state, void * a, void * b, int elementSize)`), or let your calling function prepare a single temp buffer and pass it to `mySwap`. – vgru Mar 01 '18 at 13:46
  • I have seen people unnecessarily casting `malloc()` to some pointer type, but `malloc()` returns `void *` so your cast is not only never needed, it's redundant too. Also, swapping *pointers* DOES NOT REQUIRE COPY. – Iharob Al Asimi Mar 01 '18 at 13:48
  • @IharobAlAsimi.: Because it is done implicitly no use wasting those few keystrokes writing `(any_type *)`. – user2736738 Mar 01 '18 at 13:49
  • @coderredoc Not just that, you should read the most famous question in the [tag:c] tag. – Iharob Al Asimi Mar 01 '18 at 13:49
  • @PaulOgilvie The only size involved is `elementSize` , which seems rather constant to me. – joop Mar 01 '18 at 13:49
  • @IharobAlAsimi.: Yes I read it ... :) – user2736738 Mar 01 '18 at 13:50
  • Your function will be slow as molasses due to one dynamic allocation per swap. Find a way to do without. – n. m. could be an AI Mar 01 '18 at 13:50
  • @joop, in the context of sorting you are right. As a generic swap routine, the code is fine. – Paul Ogilvie Mar 01 '18 at 13:52
  • @Franek, and how would you `free` that after `sort` called your swap the last time? (i.e. how do you know when to free it) Better have some function higher up the call tree allocate and deallocate that single swap buffer. – Paul Ogilvie Mar 01 '18 at 13:57
  • If it's OK to abort the program, it's also OK to abort it without freeing anything. Dead programs don't create memory leaks. – n. m. could be an AI Mar 01 '18 at 14:02
  • One way to avoid using `malloc()` and `free()` in every swap is to use a fixed size array (eg `char buff[64];`) and swap using that unless it is too small. Tune your buffer size so that most of the time you don’t have to allocate per swap. Consider whether `char temp[elementSize];` (a VLA) works for you. – Jonathan Leffler Mar 01 '18 at 15:04

4 Answers4

3

assert is typically used during debugging to identify problems/errors that should never occur.

Out of memory is something that can occur, and so either should not be handled by assert, or, if you do use assert, beware that it will abort the program. Once the program aborts, all memory used by the program is deallocated, so don't worry about that.

Note: If you don't want to have unwieldy if statements everywhere just to handle errors that hardly ever occur, you can use setjmp/longjmp to return to a recoverable state.

Paul Ogilvie
  • 25,048
  • 4
  • 23
  • 41
1

You have to realize that the reason malloc fails is because your computer has ran out of memory. From that point and onwards, there's nothing meaningful that your program can do, except terminating as gracefully as you can.

The OS will free the memory upon program termination, so that's not something you need to worry about.

Still, in the normal case, it is of course good practice to free() yourself, manually. Not so much for the sake of "making the memory available again" - the OS will ensure that - but to verify that your program has not gone terribly wrong along the way and created heap corruption, leaks or other bugs. If you have such bugs in your program, it will crash during the free() call, which is a good thing, as the bugs will surface.

assert should preferably not be used in production code. Build your own error handling if needed, that's something better than just violently terminating your own program in the middle of execution.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • 1
    Particularly don't use assert() on MS Windows, where this will cause some dumb OS mechanic to start searching the internet for a solution, to why you, the programmer of some unknown program, called assert. Why Microsoft thinks they can find the reason for this on the internet, I have no idea. So far, Windows has _never_ found a single "solution to this problem", but that won't prevent it from trying, and spamming 100% useless message boxes in your face. – Lundin Mar 01 '18 at 14:47
1

Avoid the problem by not using malloc.

Instead of allocating a block of memory for every swap, do the swap one byte at a time;

for (int i = 0; i < elementSize; ++i) {
  char tmp = ((char*)p1)[i];
  ((char*)p1)[i] = ((char*)p2)[i];
  ((char*)p2)[i] = tmp;
}
rici
  • 234,347
  • 28
  • 237
  • 341
  • Yes, changing the algorithm to use a fixed amount of automatic memory without taking longer is the best, if possible, like in the exact situation given. Might not work in every case though. – Deduplicator Mar 01 '18 at 20:01
  • @deduplicator: in what case would it fail? (Keeping in mind that the question is tagged [tag:c] and the C standard allows byte-by-byte copies.) – rici Mar 01 '18 at 20:04
  • Well, maybe in reality he doesn't need to do a swap, but some other ridiculously complicated calculation. Still got a +1 because if it is a swap or something like that, you had the best idea and all else is over-engineered and error-prone for no gain. – Deduplicator Mar 01 '18 at 20:08
  • Ah, I suppose you mean a hypothetical library function which requires temporary storage for purposes other than swapping values. That seems a bit distant from the question as actually asked though. – rici Mar 01 '18 at 20:08
  • @deduplicator. Sure, that's possible. Stdio has to allocate a buffer for every buffered FILE (and the FILE struct itself), and that could certainly fail. But unlike `qsort`, `fopen`'s API has a way to report failure. – rici Mar 01 '18 at 20:12
0

Only use assert() to catch programmer-error during development, in release-builds it doesn't do anything. If you need to test other things, use proper error-handling, whether that means abort(), return-codes or emulating exceptions using setjmp()/longjmp().

As an aside, do not cast the result of malloc().

Deduplicator
  • 44,692
  • 7
  • 66
  • 118
  • @Lundin: It's no doubt at least very cumbersome. Yes, for that reason normally a very bad idea, but *can* be very elegant. – Deduplicator Mar 01 '18 at 19:56