C Memory Management Issue

Question

I have traced an EXC_BAD_ACCESS to the following allocation and deallocation of memory. It involves the accelerate framework in Xcode. The main issue is that this code is in a loop. If i force the loop to only iterate once then it works fine. But when it loops (7 times) it causes an error on the second iteration. Does any of this look incorrect?

EDIT: *added actual code. This segment runs if I remove certain parts and such but seems to have poor memory management which results in issues

#import <Foundation/Foundation.h>
#include <math.h>
#include <Accelerate/Accelerate.h>

  for(int i = 0; i < 8; i++)
    {

   int XX[M][m];  //M and m are just 2 ints

   for(int kk = 0; kk < M; kk++)
            {
                for (int kk1 = 0; kk1 < m; kk1++)
                {
                    XX[kk][kk1] = [[x objectAtIndex: (kk + kk1 * J)] intValue];  //x is a NSMutableArray of NSNumber objects
                }

            }

            double FreqRes = (double) freqSamp/n;

            NSMutableArray *freqs = [[NSMutableArray alloc] initWithCapacity: round((freqSamp/2 - FreqRes) - 1)];

            int freqSum = 0;

            for(double i = -1 * freqSamp/2; i < (freqSamp/2 - FreqRes); i+= FreqRes)
            {

                [freqs addObject: [NSNumber numberWithInt: i]];

                if(i == 0)
                {
                    freqSum++;
                }

            }

            int num = [x count];
            int log2n = (int) log2f(num);
            int nOver2 = n / 2;

            FFTSetupD fftSetup = vDSP_create_fftsetupD (log2n, kFFTRadix2);

            double ffx[num];

            DSPDoubleSplitComplex fft_data;
            fft_data.realp = malloc(nOver2 * sizeof(double)); //Error usually thrown on this line in the second iteration.  Regardless of what I put there.  If I add an NSLog here it throws the error on that NSLog
            fft_data.imagp = malloc(nOver2 * sizeof(double));


            for (int i = 0; i < n; ++i)
            {
                ffx[i] = [[x objectAtIndex:i] doubleValue];
            }


            vDSP_ctozD((DSPDoubleComplex *) ffx, 2, &fft_data, 1, nOver2);

            vDSP_fft_zripD (fftSetup, &fft_data, 1, log2n, kFFTDirection_Forward);

            for (int i = 0; i < nOver2; ++i)
            {
                fft_data.realp[i] *= 0.5;
                fft_data.imagp[i] *= 0.5;
            }   

            int temp = 1;

            ffx[0] = abs(fft_data.realp[0]);
            for(int i = 1; i < nOver2; i++)
                ffx[i] = sqrt((fft_data.realp[i] * fft_data.realp[i]) + (fft_data.imagp[i] * fft_data.imagp[i]));
            ffx[nOver2] = abs(fft_data.imagp[0]);
            for(int i = nOver2-1; i > 0; i--)
            {
                ffx[nOver2 + temp] = sqrt((fft_data.realp[i] * fft_data.realp[i]) + (fft_data.imagp[i] * fft_data.imagp[i]));
                temp++;
            }

            //clear Fxx and freqs data
            vDSP_destroy_fftsetupD(fftSetup);
            free(fft_data.imagp);
            free(fft_data.realp);
            [freqs release];
}

It really depends on what happens in the code we *don't* see, and in the implementation of `vDSP_destroy_fftsetupD()`. For example, if `vDSP_destroy_fftsetupD()` frees those two pointers, then freeing them again is the error. — Ernest Friedman-Hill, Jul 25 '12 at 00:33
Probably pointless, but checking the return value of `malloc` is good practice. If it fails, just let the application die gracefully. — pmr, Jul 25 '12 at 00:34
First of all, use a debugger. It tells you what exactly is wrong. Consider providing a minimal example reproducing a problem. If you have a crashing application, provide a stack trace (again, use a debugger). What if an error is in commented out code, after all? — , Jul 25 '12 at 00:35
What does you documentation say in regards to who/which call is responsible for deallocating that memory? Could be a double free(). — Ed S., Jul 25 '12 at 00:40
Would you please supply complete code that demonstrates the problem? Also, which version of Mac OS X are you compiling with, which version of Xcode you are using (output of “xcodebuild -version” unless you have multiple versions installed), and what is your target configuration (iOS or Mac OS X; armv6, arvm7, i386, or x86_64; version number)? — Eric Postpischil, Jul 25 '12 at 00:47
Generally, you should **not** call vDSP_create_fftsetupD and vDSP_destroy_fftsetupD in a loop. These routines are not written for performance. The intended use is to create an FFTSetupD object once and use it with multiple FFT calls. (So the FFT calls are in a loop, and the setup and destroy are outside the loop.) However, multiple creates and destroys in a loop should not cause a segmentation fault, so we would be interested in debugging this if it is an Accelerate problem. (I am the principal author of the FFT routines in Accelerate.) — Eric Postpischil, Jul 25 '12 at 00:50
@ErnestFriedman-Hill: vDSP_destroy_fftsetupD has no knowledge of the pointers in fft_data and therefore cannot free them. — Eric Postpischil, Jul 25 '12 at 00:55
@EdS.: The user is responsible for managing the memory in the DSPDoubleSplitComplex object (fft_data in this code), and the malloc and free look correct, except that the malloc return value is not checked. The vDSP_destroy_fftsetupD is responsible for freeing the memory allocated by vDSP_create_fftsetupD (and not responsible for anything else). — Eric Postpischil, Jul 25 '12 at 00:58
Okay so it is definitely not just an issue with the vDSP stuff. There seems to be an overall memory management issue. If i make a simple loop with only FFT info over a small array it runs fine. I updated my code to provide more accurate info — MrHappyAsthma, Jul 25 '12 at 02:24
The additional code helps, and you are close to complete runnable code—you just need to add some #include directives and a main routine to define and initialize things used in the loop. Doing so would help hugely. I added a few things to your code to produce a runnable example, and it does not crash (running as x86_64 code on Mac OS X 10.6.8). So it is likely there is something about the program state when entering the code you have shown that causes the crash. Among other things, I would print n and log2n before the vDSP_create_fftsetupD call. — Eric Postpischil, Jul 25 '12 at 12:53
Note that n is used in `double FreqRes = (double) freqSamp/n;` but later changed in `int n = [x count];`, so I wonder if it has the value you expect. — Eric Postpischil, Jul 25 '12 at 12:54
Do you know how I would go about trying to fix this program state issue? Id add code for M and m and x but they change throughout the duration of the first couple hundred lines of code. The calculations I am doing are relatively code intensive. — MrHappyAsthma, Jul 25 '12 at 12:56
Also, that shouldn't be an issue because n is already set to the length of X in prior code, but I changed the second "int n" variable name to ensure that there is no confuse. — MrHappyAsthma, Jul 25 '12 at 12:58
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/14401/discussion-between-mrhappyasthma-and-eric-postpischil) — MrHappyAsthma, Jul 25 '12 at 13:14

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

2

Your problem could be that you are casting malloc to a value. As you're tagging this c, I'm assuming that you are compiling in c in which case you should see this answer to a previous question as to why casting with malloc is bad:

https://stackoverflow.com/a/1565552/1515720

you can get an unpredictable runtime error when using the cast without including stdlib.h.

So the error on your side is not the cast, but forgetting to include stdlib.h. Compilers may assume that malloc is a function returning int, therefore converting the void* pointer actually returned by malloc to int and then to your your pointer type due to the explicit cast. On some platforms, int and pointers may take up different numbers of bytes, so the type conversions may lead to data corruption.

Regardless though, as the answer says, YOU SHOULD NOT BE CASTING MALLOC RETURNS, because void*'s are safely implicitly converted to whatever you are assigning it to.

As another answerer stated:

vDSP_destroy_fftsetupD(fftSetup);

Could be also free'ing the memory you allocated on accident.

edited Jun 20 '20 at 09:12

Community

1
1

answered Jul 25 '12 at 00:35

ardent

2,453
1
16
15

God thank you. Even if this isn't the problem (it would throw a warning, or should at least), I don't understand why people insist on casting the retun value of malloc in C. Well, I do I suppose; they are learning from people/resources which are probably using the C parts of C++ – Ed S. Jul 25 '12 at 00:37
I got this code from the web and it worked in a smaller example with no errors so I just use it in my current code. How can I fix the casting issue. (im reading into the vDSP stuff now too) – MrHappyAsthma Jul 25 '12 at 00:43
@MrHappyAsthma, you should just remove the cast from the mallocs. I'm assuming that fft_data is a struct and that it's data types are already double*'s. So it's safe to just use malloc without the cast. – ardent Jul 25 '12 at 00:44
I'm upvoting for that. It works without the cast. But the other one solved my loop issue so it got the correct answer. Thanks! – MrHappyAsthma Jul 25 '12 at 00:48

score 2 · Answer 2 · answered Jul 25 '12 at 00:37

2

Any chance the destructor of DSPDoubleSplitComplex is freeing up those two allocated blocks?

It could also be that you are only allowed to call vDSP_create_fftsetupD and vDSP_destroy_fftsetupD once during your process's lifetime

answered Jul 25 '12 at 00:37

YePhIcK

5,816
2
27
52

try running the code in a debugger or just reading the source code (or documentation?) of the classes you are using - both could be productive exercises for you – YePhIcK Jul 25 '12 at 00:40
DSPDoubleSplitComplex is plain old data (POD) and does not have a destructor. It is implemented with C code with no C++ features. – Eric Postpischil Jul 25 '12 at 00:42
If i move the vDSPs outside of the loop then it runs. Thank you!! – MrHappyAsthma Jul 25 '12 at 00:47
@Eric Thanks for the info. I couldn't tell it was just a POD from the sample code that was provided – YePhIcK Jul 25 '12 at 00:54
@MrHappyAsthma: I am glad moving the create and the destroy outside the loop solves your immediate problem. However, I suspect there is still an issue lurking in your code. Those routines should not cause a crash when called repeatedly, even though calling them repeatedly is unadvised. So there may be another issue such as something else in the code is overwriting a portion of memory. When the create and destroy were in the loop, it may have overwritten data in them and caused a crash. Now perhaps it is overwriting other data, and you have not seen the effects yet. – Eric Postpischil Jul 25 '12 at 01:01
So you think there may be another error outside of this data allocation? – MrHappyAsthma Jul 25 '12 at 01:01
1

I'd have to agree with Eric's suspision – YePhIcK Jul 25 '12 at 01:26
I am looking through and nothing else gives me an issue as long as I move the memory stuff outside of the loop. If i test each segment individually they work fine. When i combine them all, it does seem to be a memory overwrite error situation. I un-accepted to try to get a more definite solution incase others suffer from similar situations. – MrHappyAsthma Jul 25 '12 at 02:18

C Memory Management Issue

2 Answers2