2

I'm attempting to make an array of the structure I made called StatusItem, which looks like this:

typedef struct 
{
    char* name;
    char* index;
    int optional;
} StatusItem;

Also, as I want this array to be of any size, I am using malloc. So the array is defined as such:

StatusItem* statusItem = NULL;

(its then passed to function which retrieves all the values as follows.)

statusItem = (StatusItem*)malloc(cJSON_GetArraySize(items));

...

for (i = 0 ; i < cJSON_GetArraySize(items) ; i++)
{
    strcpy(statusItem[i].name,name->valuestring);
    strcpy(statusItem[i].index,index->valuestring);
    if(!parseInt(optional->valuestring, &statusItem[i].optional));
    {
         goto cleanup;
    }
}

There's come code that involves the cJSON library in getting the string values of name, index and optional into the variables referenced above, and they are stored in the valuestring field of those variables.

I have checked that everything involving the cJSON library works fine, and returns the correct values, but the program is unable to access or store values in the statusItems array.

Any ideas? I'm almost positive that it involves some misuse of malloc on my part.

Sled
  • 18,541
  • 27
  • 119
  • 168
Nealon
  • 2,213
  • 6
  • 26
  • 40

4 Answers4

4

1) cJSON_GetArraySize(items) returns an element count - you need the size of the object factored in: malloc(cJSON_GetArraySize(items) * sizeof(StatusItem))

2) a StatusItem structure doesn't have memory for the actual string - only a pointer to a string. You can use strdup() to allocate and copy a string.

You probably want your code to look more like:

statusItem = (StatusItem*)malloc(cJSON_GetArraySize(items) * sizeof(StatusItem));

...

for (i = 0 ; i < cJSON_GetArraySize(items) ; i++)
{
    statusItem[i].name = strdup(name->valuestring);
    statusItem[i].index = strdup(index->valuestring);
    if(!parseInt(optional->valuestring, &statusItem[i].optional));
    {
         goto cleanup;
    }
}

Of course this means that you also have to free the duplicated strings explicitly when you free the array of StatusItem objects:

// to free the statusItem array, and the various strings it refers to:

for (i = 0 ; i < cJSON_GetArraySize(items) ; i++)
{
    free(statusItem[i].name);
    free(statusItem[i].index);
}

free(statusItem);
Michael Burr
  • 333,147
  • 50
  • 533
  • 760
  • How would I go about freeing those duplicated strings? – Nealon Jun 03 '13 at 19:28
  • Only way I can think of is to free them individually, but that's hell and I don't even know where strdup stores the values – Nealon Jun 03 '13 at 19:36
  • @Nealon Yes, exactly, you free them individually, you can't do anything else. `strdup()` does a `malloc()` for the copies, so it's sufficient to `free()` its return value as well (but this all is in the man page of `strdup()`). –  Jun 03 '13 at 19:38
  • @H2CO3 but the return is stored in my malloc array, and I free that at the end anyways, so shouldn't it be taken care of? – Nealon Jun 03 '13 at 19:40
  • @Nealon You either use `malloc()` and `strcpy()` **or** `strdup()`. Not both. –  Jun 03 '13 at 19:41
  • `malloc()` and `strcpy()` dont work, as my question suggests, and the post we are commenting on would beg to differ on the second point. – Nealon Jun 03 '13 at 19:44
  • @Nealon: you only called `malloc()` for `StatusItem`. That `malloc()` allocates memory for `StatusItem`, but does not allocate for its member pointers. You also need to call `StatusItem[i].name=malloc(strlen(name->ValueString)+1);` before calling `strcpy()`. – user172818 Jun 03 '13 at 21:12
  • @user172818 That sounds like it could work, but if you look below, my answer tells how I solved the problem. – Nealon Jun 04 '13 at 12:35
  • @Nealon: yes, your solution works if you know for sure the maximum length is 32 and you do not care about wasting a little bit memory. For JSON, though, I am not convinced that 32 is enough for every file. – user172818 Jun 04 '13 at 14:11
  • @user172818 I'm implementing this for use with a command where the only valid inputs are of an enumeration or a number less than 32, so it works for this case. – Nealon Jun 04 '13 at 14:13
  • @Nealon: That's a perfectly good solution (changing the structure never occurred to me as something that would be acceptable). Just make sure that your validation never permits strings that are longer than 31 characters (element 32 is for the null terminator of the longest strings). – Michael Burr Jun 04 '13 at 14:23
2

Two misuses spotted:

  1. Don't cast the return value of malloc(), it's dangerous and superfluous.

  2. You don't allocate any memory for the members of the structure - you're strcpy()ing to uninitialized pointers, so your program invokes undefined behavior.

Edit: actually three:

malloc(cJSON_GetArraySize(items));

doesn't allocate enough memory since it's not magic and it doesn't know you're reserving sizeof(StatusItem) bytes of memory, thus you have to multiply the allocation size by sizeof(StatusItem), or even better, by sizeof(*statusItem) for safety.

Community
  • 1
  • 1
  • 1. It gives me an error without the cast. 2. how would I go about this? – Nealon Jun 03 '13 at 19:24
  • @Nealon Then compile your C code with a C compiler and **never ever try compiling C code with a C++ compiler,** 2. `statusItem[i].name = malloc(128);` (or whatever) –  Jun 03 '13 at 19:28
  • @Nealon What kind of goddarned bad C compiler are you using? In C, `void *` (the return value of `malloc()`) is implicitly compatible with any data pointer type (and under POSIX, with any function pointer type as well). –  Jun 03 '13 at 19:31
  • Visual Studio, and its set to natural – Nealon Jun 03 '13 at 19:32
  • @Nealon Correct me if I'm wrong, but that's a n IDE where C++ is the default and C is almost completely unused... –  Jun 03 '13 at 19:33
  • You're right, but I set it to compile in C for this project, that's what I meant by its set to natural – Nealon Jun 03 '13 at 19:34
0

In addition, malloc takes a number of bytes, not elements. The value passed to it must be multiplied by the size of each element.

shipr
  • 2,809
  • 1
  • 24
  • 32
0

To avoid having to use strdup() which is a little 'messier' because it leaves the freeing of the memory up to the caller instead of taking care of everything itself, I modified my existing structure as follows:

typedef struct 
{
    char name[32];
    char index[32];
    int optional;
} StatusItem;

This allows 32 bytes for the name and index, which should be more than enough. Before, the structures fields were pointing to nothing, which was causing the error when trying to copy to that location. now, there is empty (or junk) memory waiting for the string to be placed in.

This allows for strcpy() to still be used, and allows for an overall cleaner implementation.

Nealon
  • 2,213
  • 6
  • 26
  • 40