EXC_BAD_ACCESS when passing a pointer-to-pointer in a struct?

Question

I've got a c-array of CGPoints in a struct. I need to replace this array when another CGPoint is added. I'd swear I'm doing this right and it seems to work fine a few times but eventually I'll get a EXC_BAD_ACCESS. What am I missing?

Here's the struct, which I've truncated to remove a lot of items that don't pertain.

typedef struct{
    CGPoint **focalPoints;
    NSUInteger focalPointCount;
    CGRect boundingRect;
}FocalPoints;

Here's how I initialize it:

CGPoint *fPoints = (CGPoint *)malloc(sizeof(CGPoint));
FocalPoints focalInfo = {&fPoints, 0, rect};

Note that focalInfo is passed by reference to another function, like so: anotherFunction(&focalInfo).

Now here's the function that replaces the Points array with a new one:

void AddFocalPoint (CGPoint focalPoint, FocalPoints *focal){
    if (focalPoint.x == CGFLOAT_MAX) return;
    if (!CGRectContainsPoint(focal->boundingRect, focalPoint)) return;
    int origCount = focal->focalPointCount;
    int newCount = origCount + 1;
    CGPoint *newPoints = (CGPoint *) malloc((newCount) * sizeof(CGPoint));
    for (int i = 0; i < newCount; i++)
        newPoints[i] = (i < origCount) ? *focal->focalPoints[i] : focalPoint; //error occurs here
    free(*focal->focalPoints);
    *focal->focalPoints = newPoints;
    focal->focalPointCount = newCount;
}

The EXC_BAD_ACCESS error occurs in the above code on line 8: newPoints[i] = (i < origCount) ? *focal->focalPoints[i] : focalPoint;. So what exactly am I doing wrong?

I'm not sure entirely, but I think newPoints[i] is a CGPoint, whereas *focal->focalPoints[i] is a CGPoint pointer... — FrankieTheKneeMan, Mar 19 '12 at 20:45
Have you considered using a linked list or even `NSMutableArray`/`NSValue` so that you're not allocating, copying, and freeing the whole thing every time you want to add a point? — jscs, Mar 19 '12 at 20:49
The original malloc of fPoints isn't technically used. But I allocate the space because the AddFocalPoint function will call free on it, and I don't want to free space that hasn't been malloc'd. `&fPoints` add an additional pointer to fPoints. I do this because I believe that's the only way to replace c-arrays. — Aaron Hayman, Mar 19 '12 at 20:49
"The original `malloc()` of `fPoints` isn't technically used": You don't need to allocate that, then -- as long as you initialize it to `NULL`, you'll be fine. `free(NULL)` is a perfectly safe no-op. — jscs, Mar 19 '12 at 20:53
lol...oh yeah, I'd forgotten about that. Thanks, it's been a little bit since I messed with c-pointers. — Aaron Hayman, Mar 19 '12 at 20:54

score 3 · Accepted Answer · answered Mar 19 '12 at 20:43

3

This is a bit of a long shot, but maybe there's an issue with operator priority in *focal->focalPoints[i]. Have you try adding parentheses according to what you are trying to achieve ?

answered Mar 19 '12 at 20:43

ksol

11,835
5
37
64

What do you mean by parenthesis? Where would I put them? – Aaron Hayman Mar 19 '12 at 20:55
in `*focal->focalPoints[i]`, I don't know wether it's `*(focal->focalPoints[i])`, `*(focal->focalPoints)[i]`, `(*focal)->focalPoints[i]`.. I think you see what I'm trying to say here – ksol Mar 19 '12 at 20:57
Well, what do you know. You were right. I added a line `CGPoint *focalPoints = *focal->focalPoints;` and I accessed it like so: `newPoints[i] = (i < origCount) ? focalPoints[i] : focalPoint;` – Aaron Hayman Mar 19 '12 at 21:03

score 2 · Answer 2 · answered Mar 19 '12 at 20:42

2

I believe the issue comes with where GCPoint *fPoints allocated as &fPoints evaluates to an address of that ... which is no longer valid once the function exits.

(The data to which it points was allocated fine with malloc.)

answered Mar 19 '12 at 20:42

The struct is not accessed after the function exits. I pass the struct by reference to another function. When that function returns, I take the data modified in the struct and pass it to another function (all still in the function that created the struct). Then the function exits, the struct is destroyed. It is accessed in no other place. – Aaron Hayman Mar 19 '12 at 20:52
Well, there goes my idea. Good luck sorting it out :( – Mar 19 '12 at 21:10

jscs · Answer 3 · 2012-03-19T21:56:25.050

Aside from the suggestion I made in a comment, of using a linked list/NSMutableArray, my other suggestion would be that you use realloc() instead of constantly using malloc(), copying by hand, and then free()ing the old allocation.

void * realloc(void *ptr, size_t size);
The realloc() function tries to change the size of the allocation pointed to by ptr to size, and returns ptr. If there is not enough room to enlarge the memory allocation pointed to by ptr, realloc() creates a new allocation, copies as much of the old data pointed to by ptr as will fit to the new allocation, frees the old allocation, and returns a pointer to the allocated memory.

This is pretty much exactly what you are doing, but you can let the library handle it for you.

(May I also humbly suggest using the word "focal" slightly less to name variables in your function?) (Also also, I'm not really clear on why focalPoints in your struct is a pointer-to-pointer. You just want an array of structs -- a single pointer should be fine.)

Consider the following (somewhat extensive) rewrite; hope that it's helpful in some way.

typedef struct{
    CGPoint *points;    // Single pointer
    NSUInteger count;
    CGRect boundingRect;
} FocalPoints;

// Renamed to match Apple's style, like e.g. CGRectIntersectsRect()
void FocalPointsAddPoint (FocalPoints *, CGPoint);

void FocalPointsAddPoint (FocalPoints *f, CGPoint thePoint){
    if (thePoint.x == CGFLOAT_MAX) return;
    if (!CGRectContainsPoint(f->boundingRect, thePoint)) return;
    NSUInteger origCount = f->count;    // |count| is typed as NSUInteger; |origCount|
    NSUInteger newCount = origCount + 1;    // and |newCount| should be consistent
    // Greatly simplified by using realloc()
    f->points = (CGPoint *) realloc(f->points, newCount * sizeof(CGPoint));
    (f->points)[newCount-1] = thePoint;
    f->count = newCount;
}

int main(int argc, const char * argv[])
{

    @autoreleasepool {
        // Just for testing; any point should be inside this rect
        CGRect maxRect = CGRectMake(0, 0, CGFLOAT_MAX, CGFLOAT_MAX);
        // Can initialize |points| to NULL; both realloc() and free() know what to do
        FocalPoints fp = (FocalPoints){NULL, 0, maxRect};
        int i;
        for( i = 0; i < 10; i++ ){
            FocalPointsAddPoint(&fp, CGPointMake(arc4random() % 100, arc4random() % 100));
            NSLog(@"%@", NSStringFromPoint(fp.points[i]));
        }

    }
    return 0;
}

Excellent, thank you. I'm sorry I've already accepted another answer, but I did up-vote yours. First, I can't use obj-c objects because I'm using ARC, which won't allow it, so NSMutableArray was out. I didn't know about `realloc` which really did simplify stuff, thanks! The reason I was using a double-pointer for my c-array was I simply couldn't replace the array with the new one. No matter how I tried, when I exited the function the struct retained a pointer to the old array. I found the answer for that here: http://stackoverflow.com/questions/1106957/pass-array-by-reference-in-c. — Aaron Hayman, Mar 19 '12 at 22:17
Glad it was helpful. Don't worry about the accept; I've got more useless rep than I know what to do with anyways. There should be a way to get an object into a struct even with ARC (I think `__unsafe_unretained` might be all you need) but it may not be worth the headache. — jscs, Mar 19 '12 at 22:25
Yeah, that would probably work but I got this working right now so I'm not going to mess with it. Besides, I enjoyed getting back to some c. — Aaron Hayman, Mar 19 '12 at 22:57

EXC_BAD_ACCESS when passing a pointer-to-pointer in a struct?

3 Answers3