Obsolete code in cJSON_InitHooks?

[MattStillerman]

cJSON_InitHooks will reset the three global hook functions to malloc, free, and realloc. But, just a few lines above the hooks are initialized to internal_malloc, internal_free, and internal_realloc.

If a user program were to call cJSON_InitHooks(NULL), then the three global hook functions will not be reset to their original values, but to this potentially incorrect set of values.

I should also point out that when cJSON_InitHooks is used to set the allocate and deallocate hooks, then if the supplied value for either of those is NULL, the function will set the corresponding hook to malloc or free rather than their internal_ forms.

BTW, there is no way to set the reallocate hook to anything except realloc (should be internal_realloc) or NULL. Would be nice to be able to set this to some custom function.

[Tangerino]

There is no realloc support. That will lead to segmentation fault if we use an alternative memory allocator library like je-malloc library. cJSON_Hooks is not the same as the internal hooks.

typedef struct cJSON_Hooks
{
      void *(*malloc_fn)(size_t sz);
      void (*free_fn)(void *ptr);
----> void *(*reallocate)(void *pointer, size_t size); <--- We need that and the code as well
} cJSON_Hooks;

Also important to let very clear that user MUST call cJSON_Free function to free text rendered by the cJSON_Print, right?

[MattStillerman]

Hi Carlos, Thanks for your answer! I realize that cJSON_Hooks and internal_hooks are not the same. And, I quite agree that it would be good to have reallocate in cJSON_Hooks. I am pointing out a somewhat orthogonal problem: malloc and internal_malloc are not necessarily the same thing (depending on platform). And similarly for free and realloc. So, we carefully initialize the global_hooks to internal_malloc, internal_free, and internal_realloc. But then, if the user “resets” the hooks, they are then incorrectly reset to malloc, free, and realloc. On many platforms this turns out to be OK, but on some platforms it is simply wrong. If we introduce a settable reallocate hook, then the current setting mechanism has another small problem. In the cJSON_InitHooks function, if individual hook functions are NULL, that is a signal to “reset” that particular hook. BUT, we may very well want to actually set the reallocate hook to NULL if it is not implemented. So, the current InitHooks function cannot be trivially extended to cover a settable reallocate hook. Best, Matt From: Carlos Tangerino [mailto:notifications@github.com] Sent: Tuesday, February 13, 2018 2:53 AM To: DaveGamble/cJSON <cJSON@noreply.github.com> Cc: MattStillerman <matt@atc-nycorp.com>; Author <author@noreply.github.com> Subject: Re: [DaveGamble/cJSON] Obsolete code in cJSON_InitHooks? (#243) There is no realloc support. That will lead to segmentation fault if we use an alternative memory allocator library like je-malloc library. cJSON_Hooks is not the same as the internal hooks. typedef struct cJSON_Hooks { void *(*malloc_fn)(size_t sz); void (*free_fn)(void *ptr); void *(*reallocate)(void *pointer, size_t size); <--- We need that and the code as well } cJSON_Hooks; Also important to let very clear that user MUST call cJSON_Free function to free text rendered by the cJSON_Print, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#243 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/ABiosHwUCaYpudx9UbiKtTq5zZjwvyAnks5tUT95gaJpZM4SDOC-> .

[FSMaxB]

cJSON_InitHooks will reset the three global hook functions to malloc, free, and realloc. But, just a few lines above the hooks are initialized to internal_malloc, internal_free, and internal_realloc.

If a user program were to call cJSON_InitHooks(NULL), then the three global hook functions will not be reset to their original values, but to this potentially incorrect set of values.
I should also point out that when cJSON_InitHooks is used to set the allocate and deallocate hooks, then if the supplied value for either of those is NULL, the function will set the corresponding hook to malloc or free rather than their internal_ forms.

The internal_ functions have been introduced in commit d1c2e2d to work around the MSVC error "C2322 address of dllimport is not static". Since the internal_ functions do nothing other than calling malloc, free and realloc, it doesn't matter if they are used or if malloc, free and realloc are used.

BTW, there is no way to set the reallocate hook to anything except realloc (should be internal_realloc) or NULL. Would be nice to be able to set this to some custom function.

Yes, I am aware of this. But I can't fix it in cJSON_InitHooks because this would break the API and ABI. I'm working on a solution for this here though (this will allow setting realloc and also passing userdata to the alloctor (e.g. for pool allocators)).

I am pointing out a somewhat orthogonal problem: malloc and internal_malloc are not necessarily the same thing (depending on platform). And similarly for free and realloc. So, we carefully initialize the global_hooks to internal_malloc, internal_free, and internal_realloc. But then, if the user “resets” the hooks, they are then incorrectly reset to malloc, free, and realloc. On many platforms this turns out to be OK, but on some platforms it is simply wrong.

As mentioned earlier, the internal_ functions are only different when compiling with MSVC. And then they are just proxies to malloc, free and realloc. This means that you can allocate with internal_malloc and free with free without any problems.

If we introduce a settable reallocate hook, then the current setting mechanism has another small problem. In the cJSON_InitHooks function, if individual hook functions are NULL, that is a signal to “reset” that particular hook. BUT, we may very well want to actually set the reallocate hook to NULL if it is not implemented. So, the current InitHooks function cannot be trivially extended to cover a settable reallocate hook.

Setting realloc to NULL is quite easy by just making your own wrappers for malloc and free and setting them with cJSON_InitHooks. I also want to mention that realloc not being available would mean that the platform is not compliant with the C standard and cJSON doesn't support platforms that do not comply with the C standard.

[FSMaxB]

@Tangerino

Also important to let very clear that user MUST call cJSON_Free function to free text rendered by the cJSON_Print, right?

If you are the only user of cJSON in your program, and you know which allocator you set, you don't need to use cJSON_free.

cJSON_free is primarily meant for Libraries that make use of cJSON. The scenario is that the library uses cJSON but the user of the library uses cJSON as well, so the library can not know if the allocators have been changed, so it needs to use cJSON_malloc and cJSON_free to be safe.