v2.1.3

---------------------------------------------------------------------------------------------------- * added more trace2 level logging in server-status collection to help issue #141 * fixed the status handling to return job information from staging also when the certificate is not there yet. * fixed a bug in JSON handling that decremented refcounters wrongly when setting another JSON object at a key where none was allowed. Added test case.
icing · Sep 5, 2019 · 2b36ce5 · 2b36ce5
1 parent 52e4f92
commit 2b36ce5
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 15 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,10 @@
+v2.1.3
+----------------------------------------------------------------------------------------------------
+ * added more trace2 level logging in server-status collection to help issue #141
+ * fixed the status handling to return job information from staging also when the certificate
+   is not there yet.
+ * fixed a bug in JSON handling that decremented refcounters wrongly when setting another
+   JSON object at a key where none was allowed. Added test case.   
  * OCSP Stapling information in "server-status" is now sorted alphabetically.
  * add a CA column to Managed Domains table
  * urls are now displayed as links with the hostname or known name as text.

diff --git a/configure.ac b/configure.ac
@@ -14,7 +14,7 @@
 #
 
 AC_PREREQ([2.69])
-AC_INIT([mod_md], [2.1.2], [[email protected]])
+AC_INIT([mod_md], [2.1.3], [[email protected]])
 
 LT_PREREQ([2.2.6])
 LT_INIT()

diff --git a/src/md_json.c b/src/md_json.c
@@ -170,18 +170,15 @@ static apr_status_t jselect_add(json_t *val, md_json_t *json, va_list ap)
     j = jselect_parent(&key, 1, json, ap);
 
     if (!j || !json_is_object(j)) {
-        json_decref(val);
         return APR_EINVAL;
     }
 
     aj = json_object_get(j, key);
     if (!aj) {
-        aj = json_array();
         json_object_set_new(j, key, aj);
     }
 
     if (!json_is_array(aj)) {
-        json_decref(val);
         return APR_EINVAL;
     }
 
@@ -229,13 +226,11 @@ static apr_status_t jselect_set(json_t *val, md_json_t *json, va_list ap)
     j = jselect_parent(&key, 1, json, ap);
 
     if (!j) {
-        json_decref(val);
         return APR_EINVAL;
     }
 
     if (key) {
         if (!json_is_object(j)) {
-            json_decref(val);
             return APR_EINVAL;
         }
         json_object_set(j, key, val);

diff --git a/src/md_status.c b/src/md_status.c
@@ -163,8 +163,7 @@ apr_status_t md_status_get_md_json(md_json_t **pjson, const md_t *md,
         rv = job_loadj(&jobj, MD_SG_STAGING, md->name, reg, p);
         if (APR_SUCCESS == rv) {
             rv = get_staging_cert_json(&certj, p, reg, md);
-            if (APR_SUCCESS != rv) goto leave;
-            if (certj) md_json_setj(certj, jobj, MD_KEY_CERT, NULL);
+            if (APR_SUCCESS == rv) md_json_setj(certj, jobj, MD_KEY_CERT, NULL);
             md_json_setj(jobj, mdj, MD_KEY_RENEWAL, NULL);
         }
         else if (APR_STATUS_IS_ENOENT(rv)) rv = APR_SUCCESS;
@@ -175,7 +174,7 @@ apr_status_t md_status_get_md_json(md_json_t **pjson, const md_t *md,
     if (APR_SUCCESS != rv) {
         md_json_setl(rv, mdj, MD_KEY_ERROR, NULL);
     }
-    *pjson = mdj; /* we return even incomplete results, maybe someone can figure out the rest */
+    *pjson = mdj;
     return rv;
 }
 

diff --git a/src/md_version.h b/src/md_version.h
@@ -27,15 +27,15 @@
  * @macro
  * Version number of the md module as c string
  */
-#define MOD_MD_VERSION "2.1.2-git"
+#define MOD_MD_VERSION "2.1.3-git"
 
 /**
  * @macro
  * Numerical representation of the version number of the md module
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_MD_VERSION_NUM 0x020102
+#define MOD_MD_VERSION_NUM 0x020103
 
 #define MD_ACME_DEF_URL    "https://acme-v02.api.letsencrypt.org/directory"
 

diff --git a/src/mod_md_status.c b/src/mod_md_status.c
@@ -478,6 +478,7 @@ int md_domains_status_hook(request_rec *r, int flags)
     apr_array_header_t *mds;
     md_json_t *jstatus, *jstock;
 
+    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "server-status for managed domains, start");
     sc = ap_get_module_config(r->server->module_config, &md_module);
     if (!sc) return DECLINED;
     mc = sc->mc;
@@ -493,9 +494,11 @@ int md_domains_status_hook(request_rec *r, int flags)
     qsort(mds->elts, (size_t)mds->nelts, sizeof(md_t *), md_name_cmp);
 
     if (!html) {
+        ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "no-html summary");
         apr_brigade_puts(ctx.bb, NULL, NULL, "ManagedDomains: ");
         if (mc->mds->nelts > 0) {
             md_status_take_stock(&jstock, mds, mc->reg, r->pool);
+            ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "got JSON summary");
             apr_brigade_printf(ctx.bb, NULL, NULL, "total=%d, ok=%d renew=%d errored=%d ready=%d",
                                 (int)md_json_getl(jstock, MD_KEY_TOTAL, NULL), 
                                 (int)md_json_getl(jstock, MD_KEY_COMPLETE, NULL), 
@@ -509,7 +512,9 @@ int md_domains_status_hook(request_rec *r, int flags)
         apr_brigade_puts(ctx.bb, NULL, NULL, "\n"); 
     }
     else if (mc->mds->nelts > 0) {
+        ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "html table");
         md_status_get_json(&jstatus, mds, mc->reg, mc->ocsp, r->pool);
+        ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "got JSON status");
         apr_brigade_puts(ctx.bb, NULL, NULL, 
                          "<hr>\n<h3>Managed Domains</h3>\n<table class='md_status'><thead><tr>\n");
         for (i = 0; i < (int)(sizeof(status_infos)/sizeof(status_infos[0])); ++i) {
@@ -522,6 +527,7 @@ int md_domains_status_hook(request_rec *r, int flags)
 
     ap_pass_brigade(r->output_filters, ctx.bb);
     apr_brigade_cleanup(ctx.bb);
+    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "server-status for managed domains, end");
 
     return OK;
 }
@@ -569,6 +575,7 @@ int md_ocsp_status_hook(request_rec *r, int flags)
     status_ctx ctx;
     md_json_t *jstatus, *jstock;
 
+    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "server-status for ocsp stapling, start");
     sc = ap_get_module_config(r->server->module_config, &md_module);
     if (!sc) return DECLINED;
     mc = sc->mc;
@@ -609,6 +616,7 @@ int md_ocsp_status_hook(request_rec *r, int flags)
 
     ap_pass_brigade(r->output_filters, ctx.bb);
     apr_brigade_cleanup(ctx.bb);
+    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "server-status for ocsp stapling, end");
 
     return OK;
 }

diff --git a/test/test_0920_status.py b/test/test_0920_status.py
@@ -38,8 +38,8 @@ def setup_method(self, method):
     def teardown_method(self, method):
         print("teardown_method: %s" % method.__name__)
 
+    # simple MD, drive it, check status before activation
     def test_920_001(self):
-        # simple MD, drive it, check status before activation
         domain = self.test_domain
         domains = [ domain ]
         conf = HttpdConf()
@@ -67,8 +67,8 @@ def test_920_001(self):
         assert 'valid' in status
         assert 'from' in status['valid']
 
+    # simple MD, drive it, manipulate staged credentials and check status
     def test_920_002(self):
-        # simple MD, drive it, manipulate staged credentials and check status
         domain = self.test_domain
         domains = [ domain ]
         conf = HttpdConf()
@@ -96,8 +96,8 @@ def test_920_002(self):
             assert status['renewal']['scts'][1]['logid'] == '293c519654c83965baaa50fc5807d4b76fbf587a2972dca4c30cf4e54547f478'
             assert status['renewal']['scts'][1]['signed'] == 'Fri, 31 May 2019 17:06:35 GMT'
 
+    # test if switching status off has effect
     def test_920_003(self):
-        # test if switching it off works
         domain = self.test_domain
         domains = [ domain ]
         conf = HttpdConf()

diff --git a/test/unit/test_md_json.c b/test/unit/test_md_json.c
@@ -225,7 +225,7 @@ static apr_status_t str_to_json(void *value, md_json_t *json, apr_pool_t *p, voi
 
 START_TEST(json_arrays)
 {
-    md_json_t *ja, *json = md_json_create(g_pool);
+    md_json_t *ja, *json = md_json_create(g_pool), *j2 = md_json_create(g_pool);
     apr_array_header_t *a, *b;
     const char *s;
     json_t *internal;
@@ -255,6 +255,14 @@ START_TEST(json_arrays)
     ja = md_json_getj(json, "array", NULL);
     ck_assert_int_eq(ja->j->refcount, 5);
 
+    ck_assert_int_eq(j2->j->refcount, 1);
+    md_json_addj(j2, json, "array", NULL);
+    ck_assert_int_eq(j2->j->refcount, 2);
+    md_json_addj(j2, json, "array", NULL);
+    ck_assert_int_eq(j2->j->refcount, 3);
+    md_json_insertj(j2, 0, json, "array", NULL);
+    ck_assert_int_eq(j2->j->refcount, 4);
+
     internal = ja->j;
     apr_pool_clear(g_pool);
     ck_assert_int_eq(internal->refcount, 0);
@@ -281,6 +289,7 @@ START_TEST(objects)
 
     md_json_sets("test2", jb, "string2", NULL);
     ck_assert_int_eq( md_json_setj(jb, json, "object", NULL), 0 );
+    ck_assert_int_eq(jb->j->refcount, 2);
     jc = md_json_getj(json, "object", NULL);
     ck_assert_ptr_nonnull( jc );
 
@@ -297,6 +306,13 @@ START_TEST(objects)
     s = md_json_writep(json, g_pool, MD_JSON_FMT_COMPACT);
     ck_assert_str_eq(s, "{\"boolean\":true,\"long\":1,\"double\":1.0,"
                      "\"string\":\"text\"}");
+
+    /* try to set an object where none can be */
+    ck_assert_int_eq( md_json_setb(1, json, "bool", NULL), 0 );
+    ck_assert_int_eq(jb->j->refcount, 2);
+    ck_assert_int_eq( md_json_setj(jb, json, "bool", "object", NULL), APR_EINVAL );
+    ck_assert_int_eq(jb->j->refcount, 2);
+
 }
 END_TEST