>
> +static apr_status_t apr_json_decode_array(apr_json_scanner_t * self,
> + apr_array_header_t ** retval)
> +{
> + apr_status_t status = APR_SUCCESS;
> + apr_pool_t *link_pool = NULL;
> + json_link_t *head = NULL, *tail = NULL;
> + apr_size_t count = 0;
> +
> + if ((status = apr_pool_create(&link_pool, self->pool)))
> + return status;
Since the other apr_json_decode_*() functions are not thread-safe (and
shouldn't be), couldn't we (re)use a single self->ptemp here (cleared
before leaving)?
The locking potentially done by apr_pool_create() looks unnecessary,
or at least a single subpool for the mutually recursive calls to
apr_json_decode_*() would be enough.
> +
> + if (self->p >= self->e) {
> + status = APR_EOF;
> + goto out;
> + }
Before creating the subpool above maybe?
> +
> + self->level--;
> + if (self->level < 0) {
> + return APR_EINVAL;
> + }
Ditto (here moreover the subpool leaks).
Also possibly don't change the level if it's already <= 0?
> +
> + self->p++; /* toss of the leading [ */
> +
> + for (;;) {
> + apr_json_value_t *element;
> + json_link_t *new_node;
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if (*self->p == ']') {
> + self->p++;
> + break;
> + }
> +
> + if (APR_SUCCESS != (status = apr_json_decode_value(self, &element)))
> {
> + goto out;
> + }
> +
> + new_node = apr_pcalloc(link_pool, sizeof(json_link_t));
> + new_node->value = element;
> + if (tail) {
> + tail->next = new_node;
> + }
> + else {
> + head = new_node;
> + }
> + tail = new_node;
> + count++;
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if (*self->p == ',') {
> + self->p++;
> + }
> + else if (*self->p != ']') {
> + status = APR_BADCH;
> + goto out;
> + }
> + }
> +
> + {
> + json_link_t *node;
> + apr_array_header_t *array = apr_array_make(self->pool, count,
> sizeof(apr_json_value_t *));
> + for (node = head; node; node = node->next) {
> + *((apr_json_value_t **) (apr_array_push(array))) = node->value;
> + }
> + *retval = array;
> + }
Hmm, finally we may not need to count the entries in a first loop
(hence any subpool), couldn't we let the array auto-grow by itself in
the loop and do validation + add in a single pass?
> +
> + self->level++;
> +
> +out:
> + if (link_pool) {
> + apr_pool_destroy(link_pool);
> + }
> + return status;
> +}
> +
> +static apr_status_t apr_json_decode_object(apr_json_scanner_t * self,
> + apr_json_object_t ** retval)
> +{
> + apr_status_t status = APR_SUCCESS;
> +
> + apr_json_object_t *object = apr_json_object_create(self->pool);
> +
> + if (self->p >= self->e) {
> + return APR_EOF;
> + }
> +
> + self->level--;
> + if (self->level < 0) {
> + return APR_EINVAL;
> + }
Same here:
if (self->level <= 0)
return APR_EINVAL;
self->level--;
>
> + self->p++; /* toss of the leading { */
> +
> + for (;;) {
> + apr_json_value_t *key;
> + apr_json_value_t *value;
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if (*self->p == '}') {
> + self->p++;
> + break;
> + }
> +
> + if ((status = apr_json_decode_value(self, &key)))
> + goto out;
Maybe apr_json_decode_string() where we'd require/validate the
leading/trailing quotes?
> +
> + if (key->type != APR_JSON_STRING) {
> + status = APR_BADCH;
> + goto out;
> + }
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if (*self->p != ':') {
> + status = APR_BADCH;
> + goto out;
> + }
> +
> + self->p++; /* eat the ':' */
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if ((status = apr_json_decode_value(self, &value)))
> + goto out;
> +
> + apr_json_object_set(object, key, value, self->pool);
> +
> + if (self->p == self->e) {
> + status = APR_EOF;
> + goto out;
> + }
> +
> + if (*self->p == ',') {
> + self->p++;
> + }
> + else if (*self->p != '}') {
> + status = APR_BADCH;
> + goto out;
> + }
> + }
> +
> + self->level++;
> +
> + *retval = object;
> +out:
> + return status;
> +}
