12
\$\begingroup\$

I am using the jsmn JSON parser (source code) to get extract data from a JSON string. jsmn stores the data in tokens that just point to the token boundaries in the JSON string instead of copying the data. For example, jsmn will create tokens like:

  • Object [0..31]

  • String [3..7], String [12..16], String [20..23]

  • Number [27..29]

This method is used to retrieve the actual characters between those values (for String objects):

char* getTextFromJSON(const char *json)
{
    json_parser p;
    jsontok_t tokens[15];
    jsontok_t key;

    if (!json) return NULL;

    initJsonParser(&p);
    parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
    for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)
    {
        key = tokens[i];
        unsigned int length = key.end - key.start;
        char keyString[length + 1];
        memcpy(keyString, &json[key.start], length);
        keyString[length] = '\0';
        if(!strcmp(keyString, "utterance"))
        {
            key = tokens[i + 1];
            length = key.end - key.start;
            char* keyString = (char*) malloc(length + 1);
            memcpy(keyString, &json[key.start], length);
            keyString[length] = '\0';
            return keyString;
        }
    }
    return NULL;
}

Here are some JSON examples that would be thrown into the parser:

{"status":0,"id":"432eac38858968c108899cc6c3a4bade-1","hypotheses" [{"utterance":"test","confidence":0.84134156}]}

{"status":5,"id":"695118aaa3d01dc2ac4aa8054d1e5bb0-1","hypotheses":[]}
\$\endgroup\$
8
\$\begingroup\$

There are a few issues with getTextFromJSON involving use of pointers, code duplication and use of an inappropriate function.

Pointers first. In this code:

jsontok_t tokens[15];
jsontok_t key;
...
key = tokens[i];

you are copying the token when you could be using a pointer:

jsontok_t *key = &tokens[i];

Copying is obviously more costly than pointing. Also note that it is often best to define the variable at its first point of use.


There are two points of duplication, one determining the size of the token array and one concerned with copying the string. Instead of this:

jsontok_t tokens[15];
...
parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)

I would do either:

#define N_TOKENS 15
jsontok_t tokens[N_TOKENS];
...
parseJson(&p, json, tokens, N_TOKENS);
for(unsigned int i = 0; i < N_TOKENS; ++i)

or

jsontok_t tokens[15];
#define N_TOKENS (sizeof tokens / sizeof tokens[0])  // or const int N_TOKENS inseatd of #define 
...etc 

The string copying code-duplication is actually unnecessary if you use a better standard library function...


Instead of using strcmp to compare the string (which involves constructing the string because it lacks a \0):

    if(!strcmp(keyString, "utterance"))

use memcmp. And you can use strndup to create a copy of the key. If you don't have strndup which is non-standard, you can easily write it (in a separate function).

These changes wont speed it up much, but are certainly cleaner:

char* getTextFromJSON(const char *json)
{
    if (!json) return NULL;

    json_parser p;
    #define N_TOKENS 15  // this normally would be at the start of the file
    jsontok_t tokens[N_TOKENS];

    initJsonParser(&p);
    int n = parseJson(&p, json, tokens, N_TOKENS);
    for (int i = 0; i < n; ++i) {
        jsontok_t *key = &tokens[i];
        if (!memcmp("utterance", &json[key->start], (size_t) (key->end - key->start))) {
            ++key;
            return strndup(&json[key->start], (size_t)(key->end - key->start));
        }
    }
    return NULL;
}
\$\endgroup\$
  • \$\begingroup\$ +1 for all the great suggestions, but the empty JSON gives a "Segmentation fault" because no utterance exists in the JSON. How would I go about fixing that? Once it is fixed I will mark it as accepted. \$\endgroup\$ – syb0rg Jul 25 '13 at 20:41
  • \$\begingroup\$ So if the utterance is not found the function returns NULL to the caller (eg in main). You need to test for that in main, so something like if ((text = getTextFromJSON(json)) != NULL) { /* do something with text */ }. Hope that helps :-) \$\endgroup\$ – William Morris Jul 25 '13 at 21:15
  • \$\begingroup\$ Right now I'm doing it with a ternary operator: printf("Text: %s", text ? text : "No text recognized."), but it still segfaults. \$\endgroup\$ – syb0rg Jul 25 '13 at 21:19
  • \$\begingroup\$ Upon changing the last return NULL to return "No text recognized", it still segfaults, odd. \$\endgroup\$ – syb0rg Jul 25 '13 at 21:27
  • 1
    \$\begingroup\$ Do you know exactly where it segfaults - have you run it in a debugger? \$\endgroup\$ – William Morris Jul 25 '13 at 21:36
6
\$\begingroup\$

Improvements

Check if parseJson() returns an error code.

int err = parseJson(&p, json, tokens, TOKEN_NUMBER);
if (err)
{
       fprintf(stderr, "Error parsing JSON: %d\n", err);
       return NULL;
}

The code initializes all of the structures, but ->start and ->end will equal -1, which is why memcmp() will fail sometimes.

for (i = parser->toknext; i < num_tokens; i++) {
    jsmn_fill_token(&tokens[i], JSMN_PRIMITIVE, -1, -1);
}

Checking for a -1 value in ->start or ->end should be sufficient.

if ((key->end - key->start) < 0) return NULL;

The tokens[] array is uninitialized before it is passed to parseJson(), so once you iterate beyond the last token, the code is trying to run memcmp() on uninitialized nonsense address values. Initialize tokens[] to something and then check for that initialization value in the start/end fields during in the for() loop.

memset(&tokens, 0, sizeof(tokens));

During each iteration of the loop check for length zero to see if the token is actually valid before passing it to memcmp(). Bail out of the loop if the token has length zero.

if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;

Add a test case for a blank token.

if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;

Final Code

#define TOKEN_NUMBER 15
char* getTextFromJSON(const char *json)
{
        if (!json) return NULL;

        json_parser p;
        jsontok_t tokens[TOKEN_NUMBER];
        memset(&tokens, 0, sizeof(tokens));
        initJsonParser(&p);
        int err = parseJson(&p, json, tokens, TOKEN_NUMBER);
        if (err)
        {
                fprintf(stderr, "Error parsing JSON: %d\n", err);
                return NULL;
        }
        for(int i = 0; i < TOKEN_NUMBER; ++i)
        {
                jsontok_t *key = &tokens[i];
                if ((key->end - key->start) < 0) return NULL;
                if (!memcmp("", &json[key->start], (size_t) (key->end - key->start))) return NULL;
                if (!memcmp("utterance", &json[key->start], (size_t) (key->end - key->start)))
                {
                        ++key;
                        return strndup(&json[key->start], (size_t) (key->end - key->start));
                }
        }
        return NULL;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.