Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce json_object_new_string_{ext,noalloc}(). #620

Conversation

besser82
Copy link
Contributor

@besser82 besser82 commented May 18, 2020

This PR is basically a working version of #476.


The json_object_new_string_ext() function helps to reduce code duplication, as it can be easily extended for different ways how a new json string-object will be created from a given string buffer.


On some low memory systems, calling malloc (by the use of strdup) may cause memory fragmentation, which eventually results in out of memory situations.

Using the json_object_new_string_noalloc() function can help to solve such issues.


test_string_noalloc is a short test to make sure the json_object_new_string_noalloc() function works as expected.

@besser82 besser82 force-pushed the topic/besser82/json_object_new_string_ext branch from 361525b to db2331c Compare May 18, 2020 16:32
@coveralls
Copy link

coveralls commented May 18, 2020

Coverage Status

Coverage increased (+0.2%) to 87.218% when pulling 0e43aab on besser82:topic/besser82/json_object_new_string_ext into fa6a7dc on json-c:master.

@besser82 besser82 force-pushed the topic/besser82/json_object_new_string_ext branch from db2331c to 51ef3e5 Compare May 18, 2020 17:01
This function helps to reduce code duplication, as it can be easily
extended for different ways how a new json string-object will be
created from a given string buffer.
On some low memory systems, calling malloc (by the use of strdup)
may cause memory fragmentation, which eventually results in out of
memory situations.

Using the json_object_new_string_noalloc() function can be a help
to solve such issues.
This is a short test to make sure the json_object_new_string_noalloc()
function works as expected.
@besser82 besser82 force-pushed the topic/besser82/json_object_new_string_ext branch from 51ef3e5 to 0e43aab Compare May 18, 2020 18:15
json_object.c Show resolved Hide resolved
json_object.c Show resolved Hide resolved
memcpy(&dstbuf, &s, sizeof(char *));
/* string buffer must be free'd here
to avoid memory leaks */
free(dstbuf);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't guarantee that calling free() will be valid. Just because you don't want to strdup the string doesn't mean that it's memory from malloc. It might be global memory, or local variables that you happen to know won't go out of scope before the jso does, or a chunk of memory from an allocator other than malloc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To actually support this, we'll need a different way to indicate whether the string is stored directly or not. Perhaps, since c_string.len is signed, we could use negative lengths to mean it's stored directly? hmm...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't guarantee that calling free() will be valid. Just because you don't want to strdup the string doesn't mean that it's memory from malloc. It might be global memory, or local variables that you happen to know won't go out of scope before the jso does, or a chunk of memory from an allocator other than malloc.

Then we would hit the same problem when the jso gets destroyed, which would call free() on the string buffer through json_object_string_delete() as well…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've marked all changes, that I have made localy, as resolved to keep track of them. I'll push them as soon as we have found a solution here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well sure, so the code in json_object_string_delete would need to do if (jso->o.c_string.len < 0) instead of if (jso->o.c_string.len >= LEN_DIRECT_STRING_DATA), and every other place that currently references LEN_DIRECT_STRING_DATA would need some kind of change. There are 7 of those total.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have a parameter which specifies a func ptr to the free() function that corresponds to the undelying memory-allocator? And maybe NULL to use the system default free()? For stack-allocated objects we could offer a noop-function as well…

That seems better than relying on users to specify a negative value for the buffer length…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You misunderstand me. I'm not saying that the caller would specify a negative value, it would be the json-c code that would use a negative value if it decides to store the string directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and in the no-strdup case we would never copy the string into direct storage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e.:

...
if ( !(flags & JSON_C_NEW_STRING_NO_STRDUP) &&
     (len < LEN_DIRECT_STRING_DATA))
{
    ...
    jso->o.c_string.len = -len;
    return jso;
}
...

json_object.c Show resolved Hide resolved
json_object.c Show resolved Hide resolved
@hawicz
Copy link
Member

hawicz commented Jun 28, 2023

Any interest in revisiting this? If so, feel free to re-open this PR or create a new one against the most recent code (ideally with previous comments addressed).

@hawicz hawicz closed this Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants