Re: 1.6 release?
On Fri, Aug 24, 2018 at 8:16 AM, Eric Covener wrote: > Starting a new thread as potential RM's may be filtering bugzilla emails. > > There are a lot of reports of PR62644 from solaris users of httpd, can > anyone RM? > Since I added the grit in the gears of this release, I'm happy to roll and submit a tarball for review tomorrow, unless someone else wants the experience. Sunday tarball gives us all day Mon, Tues (US holiday/workday) wrapping up on Wed on the 72 hour clock. There aren't so many changes that this should be difficult to review.
buildbot failure in on apr-x64-macosx-trunk
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/169 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] 1839859 Blamelist: minfrin BUILD FAILED: failed Test Sincerely, -The Buildbot
buildbot success in on apr-x64-macosx-trunk
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/167 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] 1839838 Blamelist: minfrin Build succeeded! Sincerely, -The Buildbot
buildbot success in on apr-trunk
The Buildbot has detected a restored build on builder apr-trunk while building . Full details are available at: https://ci.apache.org/builders/apr-trunk/builds/288 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: orcus_ubuntu Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered this build Build Source Stamp: [branch apr/apr/trunk] 1839838 Blamelist: minfrin Build succeeded! Sincerely, -The Buildbot
Re: apr_json object key doublons
On Sat, Sep 1, 2018 at 1:49 PM Graham Leggett wrote:
>
> On 01 Sep 2018, at 13:13, Yann Ylavic wrote:
>
> > So how about the attached patch too, allowing for case sensitive
> > apr_tables? :p
> > apr_json could then use apr_table_make_ex()…
>
> I like the idea of a case sensitive apr_table, it’s something that would have
> been useful to me in the past.
Yeah, quite tangential to JSON but indeed useful, I'll commit it
independently if no objection arises.
>
> Using it in the JSON code I’m still not keen on, as we need the hard security
> guarantees of the apr_hash method signature, which has just one mode of
> operation.
I'm not sure what you mean by the "hard security guarantees of the
apr_hash method signature", for me the only hard guarantee of
apr_hash_t is to not lose data, but that's also a guarantee of
apr_table :)
Speaking of JSON, the downside of apr_hash are no-doublons and the
need for klen all over the place, so for me the right structure is
(caseless-)apr_table.
> We’re also still forced to unnecessarily strndup all our keys on the way in
> to make them NUL terminated.
No we don't, in my patch apr_json_string_create() does apr_strmemdup()
only if the given len != APR_JSON_VALUE_STRING (which makes sense both
to make sure that the given string is NUL-terminated and to provide a
way to duplicate it if it's not already or the scope doesn't fit).
Then C string key version of apr_json_object_{set,add}() uses
APR_JSON_VALUE_STRING to build the json string key and
apr_table_setn/addn() to fill the table, so no duplication occurs by
default.
The way to duplicate the key for the user would be to
apr_json_object_{set,add}_ex(apr_json_string_create(key, strlen(key),
value, ...) explicitely, but that's not the "default" API.
Otherwise everything should work as before, and I like the _set/add()
semantics (borrowed from apr_table) nice w.r.t. doublons.
Regards,
Yann.
Re: apr_json object key doublons
On 01 Sep 2018, at 13:13, Yann Ylavic wrote: > So how about the attached patch too, allowing for case sensitive apr_tables? > :p > apr_json could then use apr_table_make_ex()… I like the idea of a case sensitive apr_table, it’s something that would have been useful to me in the past. Using it in the JSON code I’m still not keen on, as we need the hard security guarantees of the apr_hash method signature, which has just one mode of operation. We’re also still forced to unnecessarily strndup all our keys on the way in to make them NUL terminated. Regards, Graham —
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On Sat, Sep 1, 2018 at 1:39 PM Graham Leggett wrote: > > How about this? Works for me, the splitting/factorization I made in my (reverted) commit shouldn't be that bad, and then apr_json_object_set_ex() shouldn't be far from apr__json_object_set() ;) Thanks for the JOSE work anyway, looks very interesting! Regards, Yann.
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On 01 Sep 2018, at 13:06, Yann Ylavic wrote:
>> Looking at 1839779 and 1839755, both of these need to be reverted as they
>> break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on
>> these changes).
>
> There is probably another solution, see below.
>
>>
>> Please can you discuss these changes first before making them, I’ve had my
>> head buried in these RFCs for the past while and I can explain the design
>> choices behind the API, and add any documentation that’s lacking to clarify
>> how this works.
>
> OK, sorry about this.
No worries, it caught me just as I was done on apr_jose. Turns out there are a
whole of of security requirements in the RFCs that depend on JSON, requiring a
whole lot of strictness in base64/base64url processing, JSON processing, etc.
What this shows is I need to document apr_json.h better.
>> JSON whitespace is significant in some RFCs, specifically the JWE and JWS
>> RFCs, and there needs to be a mechanism to formally support whitespace for
>> this API to be be useful in a security sensitive environment.
>
> My changes were meant to allow simple (default) apr_json_object_set
> without having to create a json_string for the key.
> That's IMO how most users want it to be:
>apr_json_object_set(json, "somekey", apr_json__create(...));
> rather than:
>apr_json_object_set(json, apr_json_string_create("somekey"),
> apr_json__create(...));
> …
I’ve at least twice tried to make the same changes you have, and both times
stopped myself going “oh, yeah”. We definitely need a “simple” signature for
the common case as you describe.
How about this?
/**
* Associate a value with a key in a JSON object.
* @param obj The JSON object.
* @param key Pointer to the key string.
* @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
* 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.
*/
APU_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)));
/**
* Associate a value with a key in a JSON object, preserving whitespace.
* @param obj The JSON object.
* @param key Pointer to the key string, including any whitespace
* required.
* @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.
*/
APU_DECLARE(apr_status_t) apr_json_object_set_ex(apr_json_value_t *obj,
apr_json_value_t *key, apr_json_value_t *val,
apr_pool_t *pool) __attribute__((nonnull(1, 2, 4)));
Regards,
Graham
—
buildbot failure in on apr-trunk
The Buildbot has detected a new failure on builder apr-trunk while building . Full details are available at: https://ci.apache.org/builders/apr-trunk/builds/287 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: orcus_ubuntu Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered this build Build Source Stamp: [branch apr/apr/trunk] 1839819 Blamelist: minfrin BUILD FAILED: failed Build Sincerely, -The Buildbot
buildbot failure in on apr-x64-macosx-trunk
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/166 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] 1839819 Blamelist: minfrin BUILD FAILED: failed Build Sincerely, -The Buildbot
Re: apr_json object key doublons
On Sat, Sep 1, 2018 at 1:06 AM Graham Leggett wrote:
>
> 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.
Ah yes, right, I miised that.
>
> As a result, we cannot use the apr_tables.h API for this and remain compliant.
So how about the attached patch too, allowing for case sensitive apr_tables? :p
apr_json could then use apr_table_make_ex()...
Regards,
Yann.
Index: include/apr_tables.h
===
--- include/apr_tables.h (revision 1839763)
+++ include/apr_tables.h (working copy)
@@ -222,13 +222,26 @@ APR_DECLARE(char *) apr_array_pstrcat(apr_pool_t *
const apr_array_header_t *arr,
const char sep);
+#define APR_TABLE_CASELESS 0x1 /** ignore keys' case */
+
/**
* Make a new table.
* @param p The pool to allocate the pool out of
* @param nelts The number of elements in the initial table.
+ * @param flags Bitmask of APR_TABLE_* flags.
* @return The new table.
* @warning This table can only store text data
*/
+APR_DECLARE(apr_table_t *) apr_table_make_ex(apr_pool_t *p, int nelts,
+ int flags);
+
+/**
+ * Make a new table, ignoring keys' case.
+ * @param p The pool to allocate the pool out of
+ * @param nelts The number of elements in the initial table.
+ * @return The new table.
+ * @warning This table can only store text data
+ */
APR_DECLARE(apr_table_t *) apr_table_make(apr_pool_t *p, int nelts);
/**
Index: tables/apr_tables.c
===
--- tables/apr_tables.c (revision 1839763)
+++ tables/apr_tables.c (working copy)
@@ -303,9 +303,9 @@ APR_DECLARE(char *) apr_array_pstrcat(apr_pool_t *
* 4 bytes, normalized for case-insensitivity and packed into
* an int...this checksum allows us to do a single integer
* comparison as a fast check to determine whether we can
- * skip a strcasecmp
+ * skip a str[case]cmp
*/
-#define COMPUTE_KEY_CHECKSUM(key, checksum)\
+#define COMPUTE_KEY_CHECKSUM(key, checksum, nc)\
{ \
const char *k = (key); \
apr_uint32_t c = (apr_uint32_t)*k; \
@@ -325,7 +325,9 @@ APR_DECLARE(char *) apr_array_pstrcat(apr_pool_t *
c = (apr_uint32_t)*++k;\
checksum |= c; \
} \
-checksum &= CASE_MASK; \
+if ((nc)) {\
+checksum &= CASE_MASK; \
+} \
}
/** The opaque string-content table type */
@@ -341,6 +343,10 @@ struct apr_table_t {
/** Who created the array. */
void *creator;
#endif
+/** The compare function (str[case]cmp) */
+int (*cmp)(const char*, const char*);
+/** Bitmask of APR_TABLE_* flags */
+int flags;
/* An index to speed up table lookups. The way this works is:
* - Hash the key into the index:
* - index_first[TABLE_HASH(key)] is the offset within
@@ -397,7 +403,8 @@ APR_DECLARE(int) apr_is_empty_table(const apr_tabl
return ((t == NULL) || (t->a.nelts == 0));
}
-APR_DECLARE(apr_table_t *) apr_table_make(apr_pool_t *p, int nelts)
+APR_DECLARE(apr_table_t *) apr_table_make_ex(apr_pool_t *p, int nelts,
+ int flags)
{
apr_table_t *t = apr_palloc(p, sizeof(apr_table_t));
@@ -406,9 +413,21 @@ APR_DECLARE(int) apr_is_empty_table(const apr_tabl
t->creator = __builtin_return_address(0);
#endif
t->index_initialized = 0;
+if (flags & APR_TABLE_CASELESS) {
+t->cmp = strcasecmp;
+}
+else {
+t->cmp = strcmp;
+}
+t->flags = flags;
return t;
}
+APR_DECLARE(apr_table_t *) apr_table_make(apr_pool_t *p, int nelts)
+{
+return apr_table_make_ex(p, nelts, APR_TABLE_CASELESS);
+}
+
APR_DECLARE(apr_table_t *) apr_table_copy(apr_pool_t *p, const apr_table_t *t)
{
apr_table_t *new = apr_palloc(p, sizeof(apr_table_t));
@@ -428,6 +447,8 @@ APR_DECLARE(apr_table_t *) apr_table_copy(apr_pool
memcpy(new->index_first, t->index_first, sizeof(int) * TABLE_HASH_SIZE);
memcpy(new->index_last, t->index_last, sizeof(int) * TABLE_HASH_SIZE);
new->index_initialized = t->index_initialized;
+new->flags = t->flags;
+new->cmp = t->cmp;
return new;
}
@@ -438,6 +459,9 @@ APR_DECLARE(apr_table_t *) apr_table_clone(apr_poo
apr_table_t *new = apr_table_make(p, array->nelts);
int i;
+new->flags = t->flags;
+new->cmp = t->cmp;
+
for (i = 0; i < array->nelts; i++) {
apr_table_add(new, elts[i].key, elts[i].val);
}
@@ -483,13 +507,13 @@ APR_DECLARE(const char *) apr_table_get
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On Sat, Sep 1, 2018 at 11:53 AM Graham Leggett wrote:
>
> On 31 Aug 2018, at 19:56, yla...@apache.org wrote:
>
> Author: ylavic
> Date: Fri Aug 31 17:56:40 2018
> New Revision: 1839779
>
> URL: http://svn.apache.org/viewvc?rev=1839779&view=rev
> Log:
> apr_json: follow up to r1839755: preserve formatting when decoding object
> valueT
>
> The object key-value pair parsed by the JSON decoder contained pre/post spaces
> which we lost by changing the key type (to a C string) in
> apr_json_object_set().
>
> Add a new/private apr__json_object_set() helper taking an apr_json_value_t key
> and use it in both apr_json_object_set() and apr_json_decode_object().
>
>
> Looking at 1839779 and 1839755, both of these need to be reverted as they
> break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on
> these changes).
There is probably another solution, see below.
>
> Please can you discuss these changes first before making them, I’ve had my
> head buried in these RFCs for the past while and I can explain the design
> choices behind the API, and add any documentation that’s lacking to clarify
> how this works.
OK, sorry about this.
>
> JSON whitespace is significant in some RFCs, specifically the JWE and JWS
> RFCs, and there needs to be a mechanism to formally support whitespace for
> this API to be be useful in a security sensitive environment.
My changes were meant to allow simple (default) apr_json_object_set
without having to create a json_string for the key.
That's IMO how most users want it to be:
apr_json_object_set(json, "somekey", apr_json__create(...));
rather than:
apr_json_object_set(json, apr_json_string_create("somekey"),
apr_json__create(...));
...
>
> I am currently blocked behind these changes, so I’m going to revert them for
> now so that I can get the JOSE code in. We can discuss this in more detail
> once that is done.
I suppose what you want from my changes is to make the private
apr__json_object_set available in the API and use it for apr_jose
(e.g. with a different name like apr_json_object_put or something like
that).
I find apr_json_object_set with a plain C string key more friendly in
the usual case...
Regards,
Yann.
buildbot success in on apr-x64-macosx-trunk
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/165 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] 1839817 Blamelist: minfrin Build succeeded! Sincerely, -The Buildbot
buildbot failure in on apr-x64-macosx-trunk
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/164 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] 1839815 Blamelist: minfrin BUILD FAILED: failed Test Sincerely, -The Buildbot
buildbot success in on apr-x64-macosx-trunk
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/163 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] 1839813 Blamelist: minfrin Build succeeded! Sincerely, -The Buildbot
buildbot success in on apr-trunk
The Buildbot has detected a restored build on builder apr-trunk while building . Full details are available at: https://ci.apache.org/builders/apr-trunk/builds/284 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: orcus_ubuntu Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered this build Build Source Stamp: [branch apr/apr/trunk] 1839813 Blamelist: minfrin Build succeeded! Sincerely, -The Buildbot
buildbot failure in on apr-x64-macosx-trunk
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/162 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] 1839812 Blamelist: minfrin BUILD FAILED: failed Test Sincerely, -The Buildbot
buildbot failure in on apr-trunk
The Buildbot has detected a new failure on builder apr-trunk while building . Full details are available at: https://ci.apache.org/builders/apr-trunk/builds/283 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: orcus_ubuntu Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered this build Build Source Stamp: [branch apr/apr/trunk] 1839812 Blamelist: minfrin BUILD FAILED: failed Test Sincerely, -The Buildbot
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On 31 Aug 2018, at 19:56, yla...@apache.org wrote: > Author: ylavic > Date: Fri Aug 31 17:56:40 2018 > New Revision: 1839779 > > URL: http://svn.apache.org/viewvc?rev=1839779&view=rev > Log: > apr_json: follow up to r1839755: preserve formatting when decoding object > valueT > > The object key-value pair parsed by the JSON decoder contained pre/post spaces > which we lost by changing the key type (to a C string) in > apr_json_object_set(). > > Add a new/private apr__json_object_set() helper taking an apr_json_value_t key > and use it in both apr_json_object_set() and apr_json_decode_object(). Looking at 1839779 and 1839755, both of these need to be reverted as they break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on these changes). Please can you discuss these changes first before making them, I’ve had my head buried in these RFCs for the past while and I can explain the design choices behind the API, and add any documentation that’s lacking to clarify how this works. JSON whitespace is significant in some RFCs, specifically the JWE and JWS RFCs, and there needs to be a mechanism to formally support whitespace for this API to be be useful in a security sensitive environment. I am currently blocked behind these changes, so I’m going to revert them for now so that I can get the JOSE code in. We can discuss this in more detail once that is done. Regards, Graham —
