fix: fix NULL valuestring error

[Alanscut]

Fix NULL valuestring problem in cJSON_SetValuestring. This fixes #839 and CVE-2024-31755
Related issue #845

[Alanscut]

I think passing a NULL valuestring to cJSON_SetValuestring should not be treated as an error. I think we only need to handle with the problem of passing NULL to strlen. WDYT @sbvoxel?

[sbvoxel]

Honestly, I'm not sure. I haven't fully grokked the way cJSON is designed yet. Maybe my initial issue was opened in haste.

Random thoughts:

1. I'm no expert on CVEs. You could say I'm new to them. But this particular CVE seems very silly. There's nothing wrong with an API which when misused causes a crash. We could form a cycle instead and cause an infinite loop. That's a denial as well. Or we could try to prevent it, expensively, and that's a denial as well. It's just stupid to say this stuff. What matters is what outside parties can affect, not the developer himself.

2. It could be a bad idea to have a cJSON item/object of type string have an invalid valuestring. It's another edge case. Alternatively, you could clear/free the valuestring and set the type of the item/object to Invalid. But if that's not already a thing that happens in parsing or somewhere, then that would be an edge case, too. That's on my TODO to figure out, the extent of the Invalid state usage.

With the way cJSON is designed, it wouldn't be out of character to just return as is now being done.

[sbvoxel]

Yet another approach would be to replace the null with an empty string. A copy is made anyways, right? So you could just allocate space for a "" string instead of setting null. Might work well. But with the number of null checks in the codebase, it's not like it's allergic to nulls anyways.

I personally would probably be most inclined to crash on null being set but via an assert. But that's just not the style of cJSON and may go against its goals.

[Alanscut]

Yet another approach would be to replace the null with an empty string. A copy is made anyways, right? So you could just allocate space for a "" string instead of setting null. 

Personally I do not like this idea. Setting empty string when passing NULL looks weird. But my ears are always open.

I personally would probably be most inclined to crash on null being set but via an assert. But that's just not the style of cJSON and may go against its goals.

+1

I think we should focus on these:

1. Should we treat passing NULL pointer to valuestring as an error?
2. To treat it as an error, what should we do with object->object->valuestring? I prefer to leave it as it is instead of setting it to NULL.
3. To treat it as not an error, what should we do with object->object->valuestring?

[sbvoxel]

Yeah honestly, to the embarrasement of my former self, one good option is indeed to:

• consider NULL an error
• to return without altering the object, since this is often what cJSON does on error

Of course this can be surprising since the function isn't named 'TrySetValuestring' but that's just how cJSON is.

[Alanscut]

Considering setting NULL to object->valuestring also looks weird: the cjson object type is cJSON_String but the object->valuestring is NULL. This may be confused.

Yeah honestly, to the embarrasement of my former self, one good option is indeed to:

> consider NULL an error
> to return without altering the object, since this is often what cJSON does on error

+1

Maybe I should close this PR and just update the comment above the code.