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

json_pointer: extend setter & getter with printf() style arguments #287

Merged
merged 5 commits into from
Nov 24, 2016
Merged

json_pointer: extend setter & getter with printf() style arguments #287

merged 5 commits into from
Nov 24, 2016

Conversation

commodo
Copy link
Contributor

@commodo commodo commented Nov 16, 2016

Adds support for calling with 'json_pointer_get(obj, "/foo/%d/%s", &res, 0, bar)'
style args.

Makes it easier for doing more dynamic stuff/magic, without
needing to use vasprintf() externally.

Other stuff is cleanup, and extend the current json_pointer test.

Signed-off-by: Alexandru Ardelean ardeleanalex@gmail.com

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
@hawicz
Copy link
Member

hawicz commented Nov 19, 2016

How about switching around the arguments so the printf-style args are adjacent to the format? i.e.

int json_pointer_get(struct json_object *obj,  struct json_object **res, const char *path_format, ...);

Also, since this can result in some dangerous usage if you're not careful, please recommend always specifying a format, e.g. json_pointer_get(jo1, &jo2, "%s", "/some/path"), in the docs for the cases where you don't want to use the printf feature, and adjust the fixed-string examples/tests to call it that way.
(ditto for json_pointer_set)

@commodo
Copy link
Contributor Author

commodo commented Nov 19, 2016

I personally like the current order of args in json_pointer_get/set because it's in line with json_object_object_get/add().
I don't think we need to align too much with printf()'s way of doing things (where va_args is right after the format string).
Of course, this is more of a preference.
If you insist on changing the order of args, I don't mind doing it.

I was also thinking about not changing json_pointer_get/set functions and implement some sort of json_pointer_get_va_args() or something like that.
[EDIT] Or maybe json_pointer_get_printf() to make it really clear
That way, the change has less impact.
Thoughts on this ? Maybe it's a better approach, because it makes the user more aware that printf-ication is done behind the scenes.

Regarding always specifying a format ; hmm, I think json_pointer_get(jo1, &jo2, "%s", "/some/path") and json_pointer_get(jo1, &jo2, "/some/path") are identical in terms of danger, as long as the 3rd argument is a string constant. Typically, GCC would throw a warning/error here (when enabling -Wsecurity-format) if it's some char * variable.
I don't know if we should care much about the dangers of printf-ication [and other danger types] for users of libjson-c ; users will always find insecure ways to do things

@hawicz
Copy link
Member

hawicz commented Nov 20, 2016

Re: order of args: ok, that makes sense.

json_pointer_get(jo1, &jo2, "%s", "/some/path") and json_pointer_get(jo1, &jo2, "/some/path") are definitely not the same in terms of danger. I meant "/some/path" to include not just hard coded strings, but any arbitrary path that might be passed in.
In the first case, you're completely protected against %'s in the specified path, while in the second you can really only safely use it for an actual hard coded string, or you need to run through some escaping and/or validation steps before passing the path to json_pointer_get. It's not much of a stretch to imagine code being written where the path passed is based on user input, or other not-entirely-trustworthy data source, and providing examples that default to being safe even when that is done seems worthwhile.

Having separate printf-format-enabled and plain variants of the get and set functions sounds like a good idea. How about json_pointer_getf and json_pointer_setf, so the names aren't too long?

@commodo
Copy link
Contributor Author

commodo commented Nov 22, 2016

@hawicz So, the getf/setf variants have the params switched ( so that path_fmt is last ; was not sure how you'd prefer them ; if you want I can switch them )

These include support for printf() style args for path.

Adds support for calling with 'json_pointer_getf(obj, &res, "/foo/%d/%s", 0, bar)'
style args.

Makes it easier for doing more dynamic stuff/magic, without
needing to use vasprintf() externally.

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
…mplate

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
…s template

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
@hawicz hawicz merged commit d050f1e into json-c:master Nov 24, 2016
@ploxiln ploxiln mentioned this pull request May 10, 2017
@commodo commodo deleted the json_pointer_va_args branch June 19, 2017 06:25
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.

None yet

2 participants