Re: apr_json object key doublons

[Graham Leggett]

On 31 Aug 2018, at 22:19, Yann Ylavic  wrote:

> Actually this updated patch, rather.
> There is no more copy with an apr_table than with an apr_hash (we
> still point to the given key).
> I also added apr_json_object_add() for explicitely allowing doublons,
> while apr_json_object_set() still replaces as before, similarly to
> apr_table_add() vs apr_table_set().
> 
> WDYT?

To fill you in, the purpose of the apr_json.h API is to be RFC8259 compliant, 
which in turn is being used an implementation of JOSE support for APR 
(RFC7515/RFC7516/RFC7519), which in turn depends on a fully RFC compliant 
version of a JSON encode/decoder, meeting specific security requirements.

In the case of the apr_table.h API, keys are case insensitive, meaning that if 
we use apr_tables.h in turn apr_json.h will in turn no longer be RFC compliant 
(object keys will fail comparisons), and therefore neither will any of the JOSE 
code.

As a result, we cannot use the apr_tables.h API for this and remain compliant.

The JOSE code was ready earlier today, but the API changes have meant I need to 
update the code to the new API before being able to commit anything (you just 
beat me to it). Will do that tomorrow when I have had some sleep.

Regards,
Graham
—

Re: apr_json object key doublons

[Yann Ylavic]

[CC dev@ this time]

On Fri, Aug 31, 2018 at 3:20 PM Yann Ylavic  wrote:
>
> On Fri, Aug 31, 2018 at 2:41 PM Graham Leggett  wrote:
> >
> > On 31 Aug 2018, at 14:26, Yann Ylavic  wrote:
> >
> > > I wonder if we shouldn't change the apr_hash_t (currently used to
> > > handle JSON objects) to an apr_table_t, such that key doublons are not
> > > an issue (this isn't one in JSON specification AFAICT).
> > >
> > > Then we could get rid of 'klen' handling in several places (NUL
> > > terminated string is fine for keys IMHO, the question is more about
> > > strdup or not) while APR_JSON_FLAGS_STRICT would still do its job for
> > > overlays (and could be extended to apr_json_object_set w/ something
> > > like APR_JSON_FLAGS_MULTI, mutually exclusive).
> >
> > I wondered about this for a while, I’m keen to keep the code as it is, to 
> > follow the ap_hash_t interface with key and key length as closely as 
> > possible.
> >
> > When decoding JSON, the original strings are referenced in place, and this 
> > saves both time and memory. If the strings became NUL terminated we would 
> > have to strndup each one, which would slow us down and almost double the 
> > memory footprint.
> >
>
> I was thinking of something like the attached patch.
> We don't really need to duplicate if not asked to (i.e. given len ==
> APR_JSON_VALUE_STRING).
> Wouldn't that work?

Actually this updated patch, rather.
There is no more copy with an apr_table than with an apr_hash (we
still point to the given key).
I also added apr_json_object_add() for explicitely allowing doublons,
while apr_json_object_set() still replaces as before, similarly to
apr_table_add() vs apr_table_set().

WDYT?

Regards,
Yann.
Index: include/apr_json.h
===
--- include/apr_json.h	(revision 1839763)
+++ include/apr_json.h	(working copy)
@@ -28,7 +28,6 @@
 #include "apr.h"
 #include "apr_pools.h"
 #include "apr_tables.h"
-#include "apr_hash.h"
 #include "apr_strings.h"
 #include "apr_buckets.h"
 
@@ -169,7 +168,7 @@ struct apr_json_object_t {
 /** The key value pairs in the object are in this list */
 APR_RING_HEAD(apr_json_object_list_t, apr_json_kv_t) list;
 /** JSON object */
-apr_hash_t *hash;
+apr_table_t *table;
 };
 
 /**
@@ -198,12 +197,14 @@ APR_DECLARE(apr_json_value_t *) apr_json_value_cre
  *
  * @param pool The pool to allocate from.
  * @param val The UTF-8 encoded string value.
- * @param len The length of the string, or APR_JSON_VALUE_STRING.
+ * @param len The length of the string, or APR_JSON_VALUE_STRING;
+ *the former duplicates (and NUL terminates) val up to
+ *the given len, while the latter uses the pointer only.
  * @return The apr_json_value_t structure.
  */
-APR_DECLARE(apr_json_value_t *)
-apr_json_string_create(apr_pool_t *pool, const char *val,
-apr_ssize_t len) __attribute__((nonnull(1)));
+APR_DECLARE(apr_json_value_t *) apr_json_string_create(apr_pool_t *pool,
+const char *val, apr_ssize_t len)
+__attribute__((nonnull(1)));
 
 /**
  * Allocate and return a JSON array.
@@ -269,11 +270,9 @@ APR_DECLARE(apr_json_value_t *)
 __attribute__((nonnull(1)));
 
 /**
- * Associate a value with a key in a JSON object.
+ * Associate a value with a key in a JSON object, replacing doublon(s).
  * @param obj The JSON object.
  * @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- * terminated.
  * @param val Value to associate with the key.
  * @param pool Pool to use.
  * @return APR_SUCCESS on success, APR_EINVAL if the key is
@@ -281,22 +280,42 @@ APR_DECLARE(apr_json_value_t *)
  * @remark If the value is NULL the key value pair is deleted.
  */
 APR_DECLARE(apr_status_t) apr_json_object_set(apr_json_value_t *obj,
-const char *key, apr_ssize_t klen, apr_json_value_t *val,
-apr_pool_t *pool) __attribute__((nonnull(1, 2, 5)));
+const char *key, apr_json_value_t *val, apr_pool_t *pool)
+__attribute__((nonnull(1, 2, 4)));
 
 /**
+ * Associate a value with a key in a JSON object, with possible doublon(s).
+ * @param obj The JSON object.
+ * @param key Pointer to the key.
+ * @param val Value to associate with the key.
+ * @param pool Pool to use.
+ * @return APR_SUCCESS on success, APR_EINVAL if the key is
+ *   NULL or not a string, or the object is not an APR_JSON_OBJECT.
+ * @remark If the value is NULL the key value pair is deleted.
+ */
+APR_DECLARE(apr_status_t) apr_json_object_add(apr_json_value_t *obj,
+const char *key, apr_json_value_t *val, apr_pool_t *pool)
+__attribute__((nonnull(1, 2, 4)));
+
+/**
  * Look up the value associated with a key in a JSON object.
  * @param obj The JSON object.
  * @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- * terminated.
  * @return Returns NULL if the key is not present.
  */
 

buildbot success in on apr-x64-macosx-trunk

[buildbot]

The Buildbot has detected a restored build on builder apr-x64-macosx-trunk 
while building . Full details are available at:
https://ci.apache.org/builders/apr-x64-macosx-trunk/builds/161

Buildbot URL: https://ci.apache.org/

Buildslave for this Build: svn-x64-macosx-dgvrs

Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered 
this build
Build Source Stamp: [branch apr/apr/trunk] 1839779
Blamelist: ylavic

Build succeeded!

Sincerely,
 -The Buildbot

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[Eric Covener]

On Fri, Aug 31, 2018 at 12:20 PM Rainer Jung  wrote:
>
> To clarify: the CI build failures only send a summary email to the dev
> list. If you want to see the output, you need to visit the CI build page
> (link included in the failure mail) and there clock on the appropriate
> "stdio" link. That's probably from where Bill took the log snippet.

for posterity, I was getting the summaries, but hadn't seen them
forever don't see them unless I explicitly go into an apr/ label in
gmail.

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[William A Rowe Jr]

On Fri, Aug 31, 2018 at 11:20 AM, Rainer Jung 
wrote:

> To clarify: the CI build failures only send a summary email to the dev
> list. If you want to see the output, you need to visit the CI build page
> (link included in the failure mail) and there clock on the appropriate
> "stdio" link. That's probably from where Bill took the log snippet.
>

Indeed, sorry for any confusion.

Note also the summary to the list includes the svn rev picked up in time for
the build to fail, so it's usually pretty simple from dev@ traffic to know
whether
your commit might be related. Our commit activity is low enough, and the
auto
build frequent enough, to find out pretty quickly.

At this time, it doesn't appear we have 1.7.x branch integrated in CI.

Bill

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[Rainer Jung]

To clarify: the CI build failures only send a summary email to the dev 
list. If you want to see the output, you need to visit the CI build page 
(link included in the failure mail) and there clock on the appropriate 
"stdio" link. That's probably from where Bill took the log snippet.


Regards,

Rainer

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[Christophe JAILLET]

Le 31/08/2018 à 16:17, Eric Covener a écrit :

On Fri, Aug 31, 2018 at 10:04 AM Yann Ylavic  wrote:

On Fri, Aug 31, 2018 at 3:29 PM William A Rowe Jr  wrote:

I presume you are subscribed to the commits list, which broadcasts our CI 
results?

Hmm, I'm supposed to be subscribed to commits@ list but somehow I
didn't receive this one (yet?).
Or is it a special list?

I don't get it either!


Same here, I've never seen such CI mail.

Could you give details of what is it, and maybe update 
https://apr.apache.org/mailing-lists.html?


CJ

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[William A Rowe Jr]

Here's what I have, it is apparently the dev@ list...

Mailing-List: contact dev-h...@apr.apache.org; run by ezmlm
Precedence: bulk
List-Post: 
List-Help: 
List-Unsubscribe: 
List-Id: 
Delivered-To: mailing list 
Received: (qmail 60305 invoked by uid 99); 31 Aug 2018 12:33:02 -
Received: from Unknown (HELO buildbot-vm2.apache.org) (62.210.60.225)
by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 31 Aug 2018 12:33:02 +
Received: from buildbot-vm2 (localhost [127.0.0.1]) by
buildbot-vm2.apache.org (ASF Mail Server at buildbot-vm2.apache.org) with
ESMTP id D0321A0110 for ; Fri, 31 Aug 2018 12:32:58
+ (UTC)
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Date: Fri, 31 Aug 2018 12:32:58 +
Subject: buildbot failure in
  on apr-x64-macosx-trunk
From: build...@apache.org
To: dev@apr.apache.org
Message-Id: <20180831123258.d0321a0...@buildbot-vm2.apache.org>

The Buildbot has detected a new failure on builder apr-x64-macosx-trunk
while building . Full details are available at:
https://ci.apache.org/builders/apr-x64-macosx-trunk/builds/159

Buildbot URL: https://ci.apache.org/

Buildslave for this Build: svn-x64-macosx-dgvrs

Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit'
triggered this build
Build Source Stamp: [branch apr/apr/trunk] 1839755
Blamelist: ylavic

BUILD FAILED: failed Test

Sincerely,
 -The Buildbot



On Fri, Aug 31, 2018 at 9:17 AM, Eric Covener  wrote:

> On Fri, Aug 31, 2018 at 10:04 AM Yann Ylavic  wrote:
> >
> > On Fri, Aug 31, 2018 at 3:29 PM William A Rowe Jr 
> wrote:
> > >
> > > I presume you are subscribed to the commits list, which broadcasts our
> CI results?
> >
> > Hmm, I'm supposed to be subscribed to commits@ list but somehow I
> > didn't receive this one (yet?).
> > Or is it a special list?
>
> I don't get it either!
>

Re: Backports to consider for 1.6.4?

[Nick Kew]

> Pull request #5 https://github.com/apache/apr/pull/5/files
> corresponds to https://bz.apache.org/bugzilla/show_bug.cgi?id=61985
> Yann and I are neutral on dodging (fd == -1), what do others think?
> This would be a trivial one-line fix if we adopt it.
> 
>  Are there any committers with an opinion after reviewing the PR comments?

Yann comments on the Linux manpage's comments on negative fd.
I just looked on MacOS, and the (BSD) page is silent on the subject.

I guess there's no such thing as an authoritative reference?  In the absence
of such, I'd be reluctant to change this in 1.6, but open to something for 1.7.
I may comment on the bugzilla report.

-- 
Nick Kew

Re: 1.6 release?

[Nick Kew]

> On 30 Aug 2018, at 07:20, William A Rowe Jr  wrote:
> 
> This was my workaround for 1.6.x, more eyeballs and feedback welcome.

Looks fine to me.

I've dug deep in memory for that change: it was down to protecting a caller
who had fed bad inputs to apr_time_exp_get.  The broken change was
additional to the fix, as it appeared to be another case of the same thing.

Reviewing your fix, it does what that should've done and gets it right.
At least for callers who take any notice of returned errors!

-- 
Nick Kew

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[Eric Covener]

On Fri, Aug 31, 2018 at 10:04 AM Yann Ylavic  wrote:
>
> On Fri, Aug 31, 2018 at 3:29 PM William A Rowe Jr  wrote:
> >
> > I presume you are subscribed to the commits list, which broadcasts our CI 
> > results?
>
> Hmm, I'm supposed to be subscribed to commits@ list but somehow I
> didn't receive this one (yet?).
> Or is it a special list?

I don't get it either!

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[Yann Ylavic]

On Fri, Aug 31, 2018 at 3:29 PM William A Rowe Jr  wrote:
>
> I presume you are subscribed to the commits list, which broadcasts our CI 
> results?

Hmm, I'm supposed to be subscribed to commits@ list but somehow I
didn't receive this one (yet?).
Or is it a special list?

>
> testjson:  FAILED 1 of 7
> Line 62: expected something other than <{  "Image" : {"Width" : 800 ,
> "IDs" : [116, 943, 234, 38793],"Title" : "View from 15th Floor",
> "Animated" : false,"Thumbnail" : {  "Height" : 125,  "Width" : 
> 100,  "Url" : "http://www.example.com/image/481989943;},"Height" 
> : 600   }}>, but saw <{"Image": {"Width": 800 ,"IDs": [116, 943, 234, 
> 38793],"Title": "View from 15th Floor","Animated": false,"Thumbnail": 
> {"Height": 125,"Width": 100,"Url": "http://www.example.com/image/481989943;   
>  },"Height": 600   }}q�k� >
> Failed Tests   Total Fail Failed %
> ===
> testjson  7   1 14.29%

I'll look at it, thanks!

(the apr_json_value seemed to contain more than the simple string,
like formatting, hmm).

Re: svn commit: r1839755 - in /apr/apr/trunk: include/apr_json.h json/apr_json.c json/apr_json_decode.c

[William A Rowe Jr]

I presume you are subscribed to the commits list, which broadcasts our CI
results?

testjson:  FAILED 1 of 7Line 62: expected something other
than <{  "Image" : {"Width" : 800 ,"IDs" : [116, 943, 234,
38793],"Title" : "View from 15th Floor","Animated" : false,
"Thumbnail" : {  "Height" : 125,  "Width" : 100,  "Url" :
"http://www.example.com/image/481989943;},"Height" : 600
}}>, but saw <{"Image": {"Width": 800 ,"IDs": [116, 943, 234,
38793],"Title": "View from 15th Floor","Animated": false,"Thumbnail":
{"Height": 125,"Width": 100,"Url":
"http://www.example.com/image/481989943;},"Height": 600   }}q�k�
>Failed Tests   Total   FailFailed %
===
testjson7  1 14.29%



On Fri, Aug 31, 2018 at 7:25 AM,  wrote:

> Author: ylavic
> Date: Fri Aug 31 12:25:34 2018
> New Revision: 1839755
>
> URL: http://svn.apache.org/viewvc?rev=1839755=rev
> Log:
> apr_json: object keys are strings.
>
> Make this clear in the API by requiring a usual string in
> apr_json_object_set()
> and creating the apr_json_string_t from it.
>
> It's also more user friendly as otherwise apr_json_string_create() is to be
> used when setting an entry.
>
> Also, axe the 'pool' arg from apr_json_array_add() since it's not needed.
>
> Modified:
> apr/apr/trunk/include/apr_json.h
> apr/apr/trunk/json/apr_json.c
> apr/apr/trunk/json/apr_json_decode.c
>
> Modified: apr/apr/trunk/include/apr_json.h
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_
> json.h?rev=1839755=1839754=1839755=diff
> 
> ==
> --- apr/apr/trunk/include/apr_json.h (original)
> +++ apr/apr/trunk/include/apr_json.h Fri Aug 31 12:25:34 2018
> @@ -178,7 +178,7 @@ struct apr_json_object_t {
>   * Use apr_json_array_create() to allocate.
>   */
>  struct apr_json_array_t {
> -/** The key value pairs in the object are in this list */
> +/** The values in the array are in this list */
>  APR_RING_HEAD(apr_json_array_list_t, apr_json_value_t) list;
>  /** Array of JSON objects */
>  apr_array_header_t *array;
> @@ -271,8 +271,9 @@ APR_DECLARE(apr_json_value_t *)
>  /**
>   * Associate a value with a key in a JSON object.
>   * @param obj The JSON object.
> - * @param key Pointer to the key string, including any whitespace
> - *   required.
> + * @param key Pointer to the key.
> + * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
> + * terminated.
>   * @param val Value to associate with the key.
>   * @param pool Pool to use.
>   * @return APR_SUCCESS on success, APR_EINVAL if the key is
> @@ -280,20 +281,19 @@ APR_DECLARE(apr_json_value_t *)
>   * @remark If the value is NULL the key value pair is deleted.
>   */
>  APR_DECLARE(apr_status_t) apr_json_object_set(apr_json_value_t *obj,
> -apr_json_value_t *key, apr_json_value_t *val,
> -apr_pool_t *pool) __attribute__((nonnull(1, 4)));
> +const char *key, apr_ssize_t klen, apr_json_value_t *val,
> +apr_pool_t *pool) __attribute__((nonnull(1, 2, 5)));
>
>  /**
>   * Look up the value associated with a key in a JSON object.
>   * @param obj The JSON object.
>   * @param key Pointer to the key.
>   * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
> - *   terminated.
> + * terminated.
>   * @return Returns NULL if the key is not present.
>   */
> -APR_DECLARE(apr_json_kv_t *)
> -apr_json_object_get(apr_json_value_t *obj, const char *key,
> -apr_ssize_t klen)
> +APR_DECLARE(apr_json_kv_t *) apr_json_object_get(apr_json_value_t *obj,
> +const char *key, apr_ssize_t klen)
>  __attribute__((nonnull(1, 2)));
>
>  /**
> @@ -325,13 +325,12 @@ APR_DECLARE(apr_json_kv_t *) apr_json_ob
>   * Add the value to the end of this array.
>   * @param arr The JSON array.
>   * @param val Value to add to the array.
> - * @param pool Pool to use.
>   * @return APR_SUCCESS on success, APR_EINVAL if the array value is not
>   *   an APR_JSON_ARRAY.
>   */
>  APR_DECLARE(apr_status_t) apr_json_array_add(apr_json_value_t *arr,
> -apr_json_value_t *val, apr_pool_t *pool)
> -__attribute__((nonnull(1, 2, 3)));
> +apr_json_value_t *val)
> +__attribute__((nonnull(1, 2)));
>
>  /**
>   * Look up the value associated with a key in a JSON object.
>
> Modified: apr/apr/trunk/json/apr_json.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/json/apr_json.c?
> rev=1839755=1839754=1839755=diff
> 
> ==
> --- apr/apr/trunk/json/apr_json.c (original)
> +++ apr/apr/trunk/json/apr_json.c Fri Aug 31 12:25:34 2018
> @@ -129,25 +129,23 @@ apr_json_value_t *apr_json_null_create(a
>  return json;
>  }
>
> -apr_status_t apr_json_object_set(apr_json_value_t *object,
> apr_json_value_t *key,
> -  

Re: apr_json object key doublons

[Graham Leggett]

On 31 Aug 2018, at 14:26, Yann Ylavic  wrote:

> I wonder if we shouldn't change the apr_hash_t (currently used to
> handle JSON objects) to an apr_table_t, such that key doublons are not
> an issue (this isn't one in JSON specification AFAICT).
> 
> Then we could get rid of 'klen' handling in several places (NUL
> terminated string is fine for keys IMHO, the question is more about
> strdup or not) while APR_JSON_FLAGS_STRICT would still do its job for
> overlays (and could be extended to apr_json_object_set w/ something
> like APR_JSON_FLAGS_MULTI, mutually exclusive).

I wondered about this for a while, I’m keen to keep the code as it is, to 
follow the ap_hash_t interface with key and key length as closely as possible.

When decoding JSON, the original strings are referenced in place, and this 
saves both time and memory. If the strings became NUL terminated we would have 
to strndup each one, which would slow us down and almost double the memory 
footprint.

Regards,
Graham
—

buildbot failure in on apr-x64-macosx-trunk

[buildbot]

The Buildbot has detected a new failure on builder apr-x64-macosx-trunk while 
building . Full details are available at:
https://ci.apache.org/builders/apr-x64-macosx-trunk/builds/159

Buildbot URL: https://ci.apache.org/

Buildslave for this Build: svn-x64-macosx-dgvrs

Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered 
this build
Build Source Stamp: [branch apr/apr/trunk] 1839755
Blamelist: ylavic

BUILD FAILED: failed Test

Sincerely,
 -The Buildbot

apr_json object key doublons

[Yann Ylavic]

I wonder if we shouldn't change the apr_hash_t (currently used to
handle JSON objects) to an apr_table_t, such that key doublons are not
an issue (this isn't one in JSON specification AFAICT).

Then we could get rid of 'klen' handling in several places (NUL
terminated string is fine for keys IMHO, the question is more about
strdup or not) while APR_JSON_FLAGS_STRICT would still do its job for
overlays (and could be extended to apr_json_object_set w/ something
like APR_JSON_FLAGS_MULTI, mutually exclusive).

WDYT?