Basic question: C function to return pointer to malloc'ed struct

Question

About C structs and pointers...

Yesterday I wrote sort of the following code (try to memorize parts of it out of my memory):

    typedef struct {
    unsigned short int iFrames;
    unsigned short int* iTime; // array with elements [0..x] holding the timing for each frame
    } Tile;

    Tile* loadTile(char* sFile)
    {
    // expecting to declare enough space for one complete Tile structure, of which the base memory address is stored in the tmpResult pointer
    Tile* tmpResult = malloc(sizeof(Tile));

    // do things that set values to the Tile entity
    // ...

    // return the pointer for further use
    return tmpResult;
    }

    void main()
    {
    // define a tile pointer and set its value to the returned pointer (this should also be allowed in one row)
// Expected to receive the VALUE of the pointer - i.e. the base memory address at where malloc made space available
    Tile* tmpTile;
    tmpTile = loadTile("tile1.dat");

    // get/set elements of the tile
    // ...

    // free the tile
    free(tmpTile);
    }

What I see: I cán use the malloced Tile structure inside the function, but once I try to access it in Main, I get an error from Visual Studio about the heap (which tells me that something is freed after the call is returned).

If I change it so that I malloc space in Main, and pass the pointer to this space to the loadTile function as an argument (so that the function does no longer return anything) then it does work but I am confident that I should also be able do let the loadTile function malloc the space and return a pointer to that space right?!

Thanks!!

void main ... RAAAAARRRRGGGGGHHHHH

pmg
– pmg

2011-03-16 14:54:17 +00:00
Commented Mar 16, 2011 at 14:54 — pmg
– pmg, Commented Mar 16, 2011 at 14:54
please show us the part of the code with the bug in

David Heffernan
– David Heffernan

2011-03-16 14:56:21 +00:00
Commented Mar 16, 2011 at 14:56 — David Heffernan
– David Heffernan, Commented Mar 16, 2011 at 14:56

Ant · Accepted Answer · 2011-03-16 14:57:24Z

4

There's nothing wrong with what you're trying to do, or at least not from the code here. However, I'm concerned about this line:

unsigned short int* iTime; // array with elements [0..x] holding the timing for each frame

That isn't true unless you're also mallocing iTime somewhere:

Tile* tmpResult = malloc(sizeof(Tile));
tmpResult->iTime = malloc(sizeof(short) * n);

You will need to free it when you clean up:

free(tmpTile->iTime);
free(tmpTile);

answered Mar 16, 2011 at 14:57

Ant

4,9281 gold badge33 silver badges43 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Fred Foo Over a year ago

... and put the free logic in a separate function, free_tile, so you don't tie tile-handling client code to the internals of the Tile structure. +1.

Skizz · Accepted Answer · 2011-03-16 15:08:24Z

2

You are probably writing over memory you don't own. I guess that in this section:

// do things that set values to the Tile entity

you're doing this:

tmpResult->iFrames = n;

for (i = 0 ; i < n ; ++n)
{
  tmpResult->iTime [i] = <some value>;
}

which is wrong, you need to allocate separate memory for the array:

tmpResult->iTime = malloc (sizeof (short int) * n);

before writing to it. This make freeing the object more complex:

free (tile->iTime);
free (tile);

Alternatively, do this:

typedef struct {
  unsigned short int iFrames;
  unsigned short int iTime [1]; // array with elements [0..x] holding the timing for each frame
} Tile;

and malloc like this:

 tile = malloc (sizeof (Tile) + sizeof (short int) * (n - 1)); // -1 since Tile already has one int defined.

and the for loop remains the same:

for (i = 0 ; i < n ; ++n)
{
  tmpResult->iTime [i] = <some value>;
}

but freeing the tile is then just:

free (tile);

as you've only allocated one chunk of memory, not two. This works because C (and C++) does not do range checking on arrays.

edited Mar 16, 2011 at 15:08

answered Mar 16, 2011 at 15:03

Skizz

71.4k10 gold badges75 silver badges109 bronze badges

1 Comment

Ant Over a year ago

I think you mean tmpResult->iTime = malloc (sizeof (short) * n). Cool hack though! It's worth pointing out that this only works if the dynamically-sized array is the last item in the struct.

pmg · Accepted Answer · 2011-03-16 15:03:47Z

1

You code, with as little changes as I could live with, works for me:

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

typedef struct {
  unsigned short int iFrames;
  unsigned short int* iTime;
} Tile;

Tile *loadTile(char* sFile) {
  Tile *tmpResult = malloc(sizeof *tmpResult);
  if (!tmpResult) return NULL;

  /* do things that set values to the Tile entity */
  /* note that iTime is uninitialized */
  tmpResult->iFrames = 42;

  (void)sFile; /* used parameter */
  return tmpResult;
}

int main(void) {
  Tile* tmpTile;
  tmpTile = loadTile("tile1.dat");
  if (!tmpTile) return 1;

  printf("value: %d\n", tmpTile->iFrames);

  free(tmpTile);
  return 0;
}

answered Mar 16, 2011 at 15:03

pmg

109k14 gold badges132 silver badges203 bronze badges

Comments

florin · Accepted Answer · 2011-03-16 14:54:11Z

0

The code you showed looks OK, the error must be in the elided code.

answered Mar 16, 2011 at 14:54

florin

14.4k6 gold badges50 silver badges47 bronze badges

Comments

Judge Maygarden · Accepted Answer · 2011-03-16 14:55:13Z

0

Whatever problem you are having, it is not in the code shown in this question. Make sure you are not clobbering the pointer before returning it.

answered Mar 16, 2011 at 14:55

Judge Maygarden

27.8k9 gold badges86 silver badges101 bronze badges

Comments

DavidMFrey · Accepted Answer · 2011-03-16 14:56:44Z

0

This should work fine... could just be a warning from VisualStudio that you are freeing a pointer in a different function than it was malloced in.

answered Mar 16, 2011 at 14:56

DavidMFrey

1,6681 gold badge14 silver badges15 bronze badges

Comments

Lundin · Accepted Answer · 2011-03-16 14:57:51Z

0

Technically, your code will work on a C compiler. However, allocating dynamically inside functions and returning pointers to the allocated data is an excellent way of creating memory leaks - therefore it is very bad programming practice. A better way is to allocate the memory in the caller (main in this case). The code unit allocating the memory should be the same one that frees it.

Btw if this is a Windows program, main() must be declared to return int, or the code will not compile on a C compiler.

answered Mar 16, 2011 at 14:57

Lundin

220k47 gold badges282 silver badges446 bronze badges

4 Comments

Fred Foo Over a year ago

Returning malloc'd memory is a very common practice that greatly reduces coupling compared to what you suggest. Defining foo *make_foo() and free_foo is the idiomatic approach to foo construction/initialization and destruction in C.

Lundin Over a year ago

@larsmans I think we are saying the same thing here. To reduce coupling, you should do roughly like in these two examples: stackoverflow.com/questions/5309245/c-char-question/…

Fred Foo Over a year ago

It seemed to me as if you recommended Tile *p = malloc(sizeof(Tile)); init_tile(p); in main.

Lundin Over a year ago

@larsmans There is nothing wrong with that syntax either, depending on the function. If the function is "something that does things with a data buffer", then the function doing the task shouldn't concern itself with malloc(). For example, the whole of Windows is built in that way: pass a pointer and a size of a buffer to a function. Now, of course if the data is of a nature that main() needn't concern itself with it, it should be kept privately encapsulated in the module allocating the data.

Collectives™ on Stack Overflow

Basic question: C function to return pointer to malloc'ed struct

7 Answers 7

1 Comment

1 Comment

Comments

Comments

Comments

Comments

4 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

1 Comment

1 Comment

Comments

Comments

Comments

Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related