examples/jsonrpc/jsonrpc.c

[Solar Designer]

David,

JFYI:

The examples/jsonrpc/jsonrpc.c example tries to be a bit careful with
its safe_strcmp(), yet it has a NULL pointer dereference bug here:

char *str=onion_low_strdup( onion_dict_rget(jreq,"params", "str", NULL) );

/// check real start and end. To prepare to write back the stripped string.
int start=0, end=strlen(str);

when the request doesn't provide "str". onion_low_strdup() returns
NULL, and strlen() segfaults. You could want to add e.g.:

if (!str)
str = "";

before the strlen(), as a quick hack to avoid this, or better yet have
it properly report an error.

Alexander

[Solar Designer]

Oh, and it also segfaults here when there's no "id":

onion_dict_add(jres, "id", onion_dict_get(jreq, "id"), 0);

and it passes char instead of int to isspace(), and it misses
"static" for functions that are local to this file, which are
safe_strcmp() and strip_rpc().

This example could use a good cleanup, so that it would encourage better
practices.

Alexander

[Solar Designer]

... and there's more. Why does one of these lines use OD_DUP_VALUE? -

/// Prepare message
onion_dict *jres=onion_dict_new();
onion_dict_add(jres, "jsonrpc", "2.0", 0);
onion_dict_add(jres, "result", str+start, OD_DUP_VALUE);

onion_dict_add(jres, "id", onion_dict_get(jreq, "id"), 0);

"str" isn't freed until after the response is written, and str+start is
just part of it. Indeed, the program still appears to work fine (to the
extent it did before) after I changed OD_DUP_VALUE to 0 on that line.

In what cases is OD_DUP_VALUE actually needed? Is it needed when the
original string is no longer guaranteed to be available by the time of
onion_dict_to_json() or by the time of onion_block_data() or by the time
of onion_response_write()? It'd be nice to have a well-commented
example that would illustrate this kind of detail.

Alexander

[Solar Designer]

On Wed, Feb 17, 2016 at 10:21:52PM +0300, Solar Designer wrote:

> and it passes char instead of int to isspace()

Worse, src/onion/dict.c does it too. Common libc's work around this
(such as by using 384-element tables), but nevertheless it's a bug.

The c argument is an int, the value of which the application shall
ensure is a character representable as an unsigned char or equal to the
value of the macro EOF. If the argument has any other value, the behav-
ior is undefined.

A non-ASCII "char" may have "other value" (negative), so "behavior is
undefined." Additionally, ctype macros may legitimately be implemented
such that they actually assume the type is int.

We need to use something like:

int c = (unsigned char)ch;

then "c" is usable with ctype macros. Alternatively, it's
(int)(unsigned char) right on every use. Yes, this is cumbersome.

Alexander

[Zachary Grafton]

Alexander,

I submitted another pull request that should address the the issue you pointed out with the undefined behavior. I'll see if I can find the time to get to the other issues you pointed out as well.

Thanks for all the input,

Zack

[Zachary Grafton]

Alexander,

I'm not 100% sure on this, but I believe it's because the onion_dict_free function will try to free the value stored in the key. In this case it would probably lead to a double free issue when the onion_dict_free is called at the end of the block after onion_low_free is called on str. Either way the example is definitely confusing. I have been poking around the internals of onion for a couple of months and never even looked at that example. I think the example is claiming to adhere to the json rpc 2.0 specification, however, from a cursory reading of the spec, it seems that the id field is not necessarily required. The example should check for that and return a proper error message. I think that according to the specification, when the id field is missing, that should be classified as an invalid request and the connection should be terminated in this case, unless your were sending a json rpc notification.

If that's the behavior that you expect, please let me know and I'll try to fix it sometime today.

Thanks,

Zack

[Solar Designer]

Zack,

On Thu, Feb 18, 2016 at 04:49:52AM -0800, Zachary Grafton wrote:
> I submitted another pull request that should address the the issue you
> pointed out with the undefined behavior. I'll see if I can find the time to
> get to the other issues you pointed out as well.

Thank you!

I see that your pull request with the macro casting to (int)(unsigned
char) got merged. That's good. Ideally, you'd also wrap the macro
argument in braces, just in case it's used on something weirder than it
currently is.

A further thought I have, though, is that perhaps this code should have
its own hard-coded checks for specific characters instead of using ctype
macros, because those are locale-dependent (in case an application using
libonion has initialized a locale). JSON isn't supposed to be
locale-dependent.

Alexander

[Solar Designer]

Zack,

> On Wednesday, February 17, 2016 at 7:42:53 PM UTC-5, Solar Designer wrote:
> > Why does one of these lines use OD_DUP_VALUE? -
> >
> > /// Prepare message
> > onion_dict *jres=onion_dict_new();
> > onion_dict_add(jres, "jsonrpc", "2.0", 0);
> > onion_dict_add(jres, "result", str+start, OD_DUP_VALUE);
> > onion_dict_add(jres, "id", onion_dict_get(jreq, "id"), 0);
> >
> > "str" isn't freed until after the response is written, and str+start is
> > just part of it. Indeed, the program still appears to work fine (to the
> > extent it did before) after I changed OD_DUP_VALUE to 0 on that line.

On Thu, Feb 18, 2016 at 05:05:36AM -0800, Zachary Grafton wrote:
> I'm not 100% sure on this, but I believe it's because the onion_dict_free
> function will try to free the value stored in the key.

If I followed the calls correctly, onion_dict_node_data_free() would be
the lower-level functions eventually doing it, but only when
OD_FREE_VALUE is set. In this case, there doesn't appear to be anything
setting that flag. So no, while your theory was good, it doesn't appear
to apply to this example.

Also, if your theory applied, then the code would be trying to free the
constant string "2.0", and would likely be crashing right there.

> In this case it
> would probably lead to a double free issue when the onion_dict_free is
> called at the end of the block after onion_low_free is called on str.

That wouldn't exactly be a double-free, because str+start isn't even a
valid pointer to free unless the string started with no whitespace.

Anyway, the example mostly works fine (on valid input) whether with or
without the OD_DUP_VALUE. (It probably leaks memory as written with
OD_DUP_VALUE. I didn't check for memory leaks.)

> Either way the example is definitely confusing. I have been poking around
> the internals of onion for a couple of months and never even looked at that
> example. I think the example is claiming to adhere to the json rpc 2.0
> specification, however, from a cursory reading of the spec, it seems that
> the id field is not necessarily required. The example should check for that
> and return a proper error message. I think that according to the
> specification, when the id field is missing, that should be classified as
> an invalid request and the connection should be terminated in this case,
> unless your were sending a json rpc notification.

OK. In any case, it shouldn't segfault even on invalid input.

> If that's the behavior that you expect, please let me know and I'll try to
> fix it sometime today.

I am not currently playing with JSON-RPC specifically, so I can't answer
this directly. I'd need to take a look at JSON-RPC spec.

So far, I wrote a prototype server implementing an HTTP+JSON protocol
that I needed, using libonion and those dict functions, and it appears
to work fine and without memory leaks (on correct input, at least).
I dislike it having so much malloc()/free() activity, though, which is
potentially costly in a multi-threaded program. And yes, I am aware
libonion lets me hook its memory allocation; I'll consider that.

Alexander

[Solar Designer]

Zack,

Also, your patch is only for dict.c, whereas there are many more uses of
ctype macros in other source files in libonion:

$ fgrep -rl ctype.h .
./tools/otemplate/tags.c
./tools/otemplate/otemplate.c
./tools/otemplate/functions.c
./tools/common/updateassets.c
./src/onion/codecs.c
./src/onion/url.c
./src/onion/dict.c
./src/onion/handlers/webdav.c
./src/onion/request_parser.c
./src/onion/request.c
./src/onion/mime.c
./examples/jsonrpc/jsonrpc.c
./examples/otop/otop.c

Perhaps SAFETY_CAST() should be moved to a common header file and used
from all those files, or/and the checks should be made explicit where
the dependency on locale was not appropriate anyway (and this could very
well be all uses of those macros in libonion, given what it does).

Alexander

[Zachary Grafton]

Alexander,

Thanks for pointing that issue out. I think you might be right about the SAFETY_CAST macro. I'll see if I can get around to going through all that code this week.

Zack

--
You received this message because you are subscribed to the Google Groups "onion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to onion-dev+...@coralbits.com.
To post to this group, send email to onio...@coralbits.com.
Visit this group at https://groups.google.com/a/coralbits.com/group/onion-dev/.
For more options, visit https://groups.google.com/a/coralbits.com/d/optout.

[Zachary Grafton]

Alexander,

I've created a pull request that should fix these issues. You were definitely right about removing the stuff that is dependent on locale. The parser was allowing characters that aren't allowed by the specification, even in the default locale. A form feed or vertical tab isn't considered white space according to the JSON spec, and in the default locale, isspace considers that white space. I've removed all references to to ctype.h in the dict.c file and I should be getting around to reviewing the rest of the code tomorrow.

Zack

[Solar Designer]

Hi Zack,

On Wed, Mar 02, 2016 at 09:42:14PM +0000, Zachary Grafton wrote:
> I've created a pull request that should fix these issues. You were
> definitely right about removing the stuff that is dependent on locale. The
> parser was allowing characters that aren't allowed by the specification,
> even in the default locale. A form feed or vertical tab isn't considered
> white space according to the JSON spec, and in the default locale, isspace
> considers that white space. I've removed all references to to ctype.h in
> the dict.c file and I should be getting around to reviewing the rest of the
> code tomorrow.

Thank you! I notice there are still uses of ctype macros left, which
should also be dealt with. The uses of isalnum() in src/onion/codecs.c
shouldn't be locale-dependent, and the use of toupper() in
src/onion/handlers/webdav.c is easily avoided by checking against both
'T' and 't'.

There's just one use of ONION_SAFETY_CAST() left in src/onion/mime.c,
which makes having this macro pointless. BTW, I didn't mean curly
braces when I suggested putting the macro argument in braces, but now
that we're moving away from all ctype stuff anyway, I suggest that you
replace this use with non-locale-aware explicit checks for whatever
whitespace characters are supposed to be recognized in that place.

Thanks again,

Alexander

[Zachary Grafton]

Alexander,

I planned to keep working on it. Thanks for pointing out the macro, I was thinking () but typed {} because of thinking about braces. I wanted to keep the macro in case I needed it in other places where doing a locale dependent compare was acceptable. Do you know if it is considered legal for /etc/mime.types to have vertical tabs and other whitespace that is checked for by the ctypes macros? I haven't really been able to find any sort of specification for this file. I didn't want to change the behavior of parsing this file in the mime.c file until I had a chance to research it further.

Thanks,

Zack

[Solar Designer]

Zack,

On Mon, Mar 14, 2016 at 04:44:36AM -0700, Zachary Grafton wrote:
> Do you know if it is considered legal for
> /etc/mime.types to have vertical tabs and other whitespace that is checked
> for by the ctypes macros? I haven't really been able to find any sort of
> specification for this file. I didn't want to change the behavior of
> parsing this file in the mime.c file until I had a chance to research it
> further.

I don't know reliably, but I think there's no specification, and the
expectation is that only ' ' and '\t' would work as whitespace, as well
as '\n' or "\r\n" as end-of-line. I think we shouldn't allow vertical
tab and formfeed there.

Thank you for committing some further fixes in this area. Your
is_alnum() in codecs.c looks wrong to me:

static int is_alnum(char c) {
if(c >= '0' && c <= '9')
return 1;
return 0;
}

That's not "alnum". request.c has another is_alnum(), which is
probably correct.

Alexander

[Zachary Grafton]

Alexander,

I'll patch those issues up. I think when I wrote the is_alnum function, I was a little distracted by some of the shiny tools from clang and somehow lost my mind.

Zack