QLOG: Minor updates

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22037)

4 files changed, 98 insertions, 103 deletions

diff --git a/ssl/quic/json_enc.c b/ssl/quic/json_enc.c
index a95244f499..f2c6f444e6 100644
--- a/ssl/quic/json_enc.c
+++ b/ssl/quic/json_enc.c
@@ -292,11 +292,6 @@ int ossl_json_in_error(OSSL_JSON_ENC *json)
 static void json_write_qstring(OSSL_JSON_ENC *json, const char *str);
 static void json_indent(OSSL_JSON_ENC *json);
 
-static ossl_inline int json_in_error(const OSSL_JSON_ENC *json)
-{
-    return json->error;
-}
-
 static void json_raise_error(OSSL_JSON_ENC *json)
 {
     json->error = 1;
@@ -312,7 +307,7 @@ static void json_undefer(OSSL_JSON_ENC *json)
 
 static void json_write_char(OSSL_JSON_ENC *json, char ch)
 {
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     json_undefer(json);
@@ -322,7 +317,7 @@ static void json_write_char(OSSL_JSON_ENC *json, char ch)
 
 static void json_write_str(OSSL_JSON_ENC *json, const char *s)
 {
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     json_undefer(json);
@@ -350,7 +345,7 @@ static int json_pre_item(OSSL_JSON_ENC *json)
 {
     int s;
 
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return 0;
 
     switch (json->state) {
@@ -364,7 +359,7 @@ static int json_pre_item(OSSL_JSON_ENC *json)
 
         if (s == 1) {
             json_write_char(json, ',');
-            if (json_in_error(json))
+            if (ossl_json_in_error(json))
                 return 0;
 
             json_indent(json);
@@ -422,21 +417,25 @@ static void composite_end(OSSL_JSON_ENC *json, int type, char ch)
 {
     int was_defer = json->defer_indent;
 
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     json->defer_indent = 0;
 
-    if (json_peek(json) != type)
+    if (json_peek(json) != type) {
+        json_raise_error(json);
         return;
+    }
 
     if (type == 0 && json->state == STATE_PRE_ITEM) {
         json_raise_error(json);
         return;
     }
 
-    if (!json_pop(json))
+    if (!json_pop(json)) {
+        json_raise_error(json);
         return;
+    }
 
     if (!was_defer)
         json_indent(json);
@@ -477,7 +476,7 @@ void ossl_json_array_end(OSSL_JSON_ENC *json)
  */
 void ossl_json_key(OSSL_JSON_ENC *json, const char *key)
 {
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     if (json_peek(json) != 0) {
@@ -498,7 +497,7 @@ void ossl_json_key(OSSL_JSON_ENC *json, const char *key)
     }
 
     json_write_qstring(json, key);
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     json_write_char(json, ':');
@@ -587,7 +586,7 @@ void ossl_json_i64(OSSL_JSON_ENC *json, int64_t value)
         : (uint64_t)-value;
     json_u64(json, uv, /*noquote=*/1);
 
-    if (quote && !json_in_error(json))
+    if (quote && !ossl_json_in_error(json))
         json_write_char(json, '"');
 }
 
@@ -626,7 +625,7 @@ json_write_qstring_inner(OSSL_JSON_ENC *json, const char *str, size_t str_len,
     int i;
     size_t j;
 
-    if (json_in_error(json))
+    if (ossl_json_in_error(json))
         return;
 
     json_write_char(json, '"');
@@ -651,7 +650,7 @@ json_write_qstring_inner(OSSL_JSON_ENC *json, const char *str, size_t str_len,
                 obuf[0] = '\\';
                 obuf[1] = 'u';
                 for (i = 0; i < 4; ++i)
-                    obuf[2 + i] = hex_digit((c >> ((3 - i) * 4)) & 0xF);
+                    obuf[2 + i] = hex_digit((c >> ((3 - i) * 4)) & 0x0F);
                 obuf[6] = '\0';
                 o = obuf;
             } else {
diff --git a/ssl/quic/qlog.c b/ssl/quic/qlog.c
index 7815f2af39..8fa55c567e 100644
--- a/ssl/quic/qlog.c
+++ b/ssl/quic/qlog.c
@@ -58,11 +58,10 @@ QLOG *ossl_qlog_new(const QLOG_TRACE_INFO *info)
     if (qlog == NULL)
         return NULL;
 
-    qlog->info = *info;
-
-    qlog->info.title        = NULL;
-    qlog->info.description  = NULL;
-    qlog->info.group_id     = NULL;
+    qlog->info.odcid        = info->odcid;
+    qlog->info.is_server    = info->is_server;
+    qlog->info.now_cb       = info->now_cb;
+    qlog->info.now_cb_arg   = info->now_cb_arg;
 
     if (info->title != NULL
         && (qlog->info.title = OPENSSL_strdup(info->title)) == NULL)
@@ -97,16 +96,19 @@ err:
 
 QLOG *ossl_qlog_new_from_env(const QLOG_TRACE_INFO *info)
 {
-    QLOG *qlog;
+    QLOG *qlog = NULL;
     const char *qlogdir = ossl_safe_getenv("QLOGDIR");
     const char *qfilter = ossl_safe_getenv("QFILTER");
     char qlogdir_sep, *filename = NULL;
     size_t i, l, strl;
 
-    if (info == NULL || qlogdir == NULL || strlen(qlogdir) == 0)
+    if (info == NULL || qlogdir == NULL)
         return NULL;
 
     l = strlen(qlogdir);
+    if (l == 0)
+        return NULL;
+
     qlogdir_sep = ossl_determine_dirsep(qlogdir);
 
     /* strlen("client" / "server"); strlen(".sqlog"); _; separator; _; NUL */
@@ -122,29 +124,26 @@ QLOG *ossl_qlog_new_from_env(const QLOG_TRACE_INFO *info)
     for (i = 0; i < info->odcid.id_len; ++i)
         l += sprintf(filename + l, "%02x", info->odcid.id[i]);
 
-    l += snprintf(filename + l, strl, "_%s.sqlog",
+    l += snprintf(filename + l, strl - l, "_%s.sqlog",
                   info->is_server ? "server" : "client");
 
     qlog = ossl_qlog_new(info);
-    if (qlog == NULL) {
-        OPENSSL_free(filename);
-        return NULL;
-    }
+    if (qlog == NULL)
+        goto err;
 
-    if (!ossl_qlog_set_sink_filename(qlog, filename)) {
-        OPENSSL_free(filename);
-        ossl_qlog_free(qlog);
-        return NULL;
-    }
+    if (!ossl_qlog_set_sink_filename(qlog, filename))
+        goto err;
 
-    if (qfilter != NULL && !ossl_qlog_set_filter(qlog, qfilter)) {
-        OPENSSL_free(filename);
-        ossl_qlog_free(qlog);
-        return NULL;
-    }
+    if (qfilter != NULL && !ossl_qlog_set_filter(qlog, qfilter))
+        goto err;
 
     OPENSSL_free(filename);
     return qlog;
+
+err:
+    OPENSSL_free(filename);
+    ossl_qlog_free(qlog);
+    return NULL;
 }
 
 void ossl_qlog_free(QLOG *qlog)
@@ -293,7 +292,7 @@ static void qlog_event_seq_header(QLOG *qlog)
                 ossl_json_array_begin(&qlog->json);
                 {
                     ossl_json_str(&qlog->json, "QUIC");
-                }
+                } /* protocol_type */
                 ossl_json_array_end(&qlog->json);
 
                 write_str_once(qlog, "group_id", (char **)&qlog->info.group_id);
@@ -303,9 +302,9 @@ static void qlog_event_seq_header(QLOG *qlog)
                 {
                     ossl_json_key(&qlog->json, "process_id");
                     ossl_json_u64(&qlog->json, (uint64_t)getpid());
-                }
+                } /* system_info */
                 ossl_json_object_end(&qlog->json);
-            }
+            } /* common_fields */
             ossl_json_object_end(&qlog->json);
 
             ossl_json_key(&qlog->json, "vantage_point");
@@ -314,9 +313,9 @@ static void qlog_event_seq_header(QLOG *qlog)
                 ossl_json_key(&qlog->json, "type");
                 ossl_json_str(&qlog->json, qlog->info.is_server
                                     ? "server" : "client");
-            }
+            } /* vantage_point */
             ossl_json_object_end(&qlog->json);
-        }
+        } /* trace */
         ossl_json_object_end(&qlog->json);
     }
     ossl_json_object_end(&qlog->json);
@@ -380,7 +379,7 @@ int ossl_qlog_event_try_begin(QLOG *qlog,
     return 1;
 }
 
-static void qlog_event_end(QLOG *qlog, int abort)
+void ossl_qlog_event_end(QLOG *qlog)
 {
     if (!ossl_assert(qlog != NULL && qlog->event_type != QLOG_EVENT_TYPE_NONE))
         return;
@@ -389,16 +388,6 @@ static void qlog_event_end(QLOG *qlog, int abort)
     qlog->event_type = QLOG_EVENT_TYPE_NONE;
 }
 
-void ossl_qlog_event_end(QLOG *qlog)
-{
-    qlog_event_end(qlog, /*abort=*/0);
-}
-
-void ossl_qlog_event_abort(QLOG *qlog)
-{
-    qlog_event_end(qlog, /*abort=*/1);
-}
-
 /*
  * Field Generators
  * ================
@@ -625,6 +614,9 @@ static void filter_apply(size_t *enabled, int add,
 
 static int lex_fail(struct lexer *lex, const char *msg)
 {
+    /*
+     * TODO(QLOG): Determine how to print log messages about bad filter strings
+     */
     lex->p = lex->term_end = lex->end;
     return 0;
 }
diff --git a/ssl/quic/qlog_event_helpers.c b/ssl/quic/qlog_event_helpers.c
index 16fd8ae74e..fd1d64af21 100644
--- a/ssl/quic/qlog_event_helpers.c
+++ b/ssl/quic/qlog_event_helpers.c
@@ -203,8 +203,8 @@ static void ignore_res(int x) {}
  * goes wrong before then) while avoiding the need to keep around bookkeeping
  * data on what frames it contained.
  */
-static int log_frame(QLOG *qlog_instance, PACKET *pkt,
-                     uint64_t *need_skip)
+static int log_frame_actual(QLOG *qlog_instance, PACKET *pkt,
+                            uint64_t *need_skip)
 {
     uint64_t frame_type;
     OSSL_QUIC_FRAME_ACK ack;
@@ -232,7 +232,6 @@ static int log_frame(QLOG *qlog_instance, PACKET *pkt,
             goto unknown;
 
         QLOG_STR("frame_type", "ping");
-        QLOG_U64("length", 1);
         break;
     case OSSL_QUIC_FRAME_TYPE_ACK_WITHOUT_ECN:
     case OSSL_QUIC_FRAME_TYPE_ACK_WITH_ECN:
@@ -451,7 +450,8 @@ static int log_frame(QLOG *qlog_instance, PACKET *pkt,
             uint64_t stream_id, x;
 
             if (!ossl_quic_wire_decode_frame_stream_data_blocked(pkt,
-                                                                 &stream_id, &x))
+                                                                 &stream_id,
+                                                                 &x))
                 goto unknown;
 
             QLOG_STR("frame_type", "stream_data_blocked");
@@ -511,13 +511,26 @@ unknown:
     return 1;
 }
 
+static void log_frame(QLOG *qlog_instance, PACKET *pkt,
+                      uint64_t *need_skip)
+{
+    size_t rem_before, rem_after;
+
+    rem_before = PACKET_remaining(pkt);
+
+    if (!log_frame_actual(qlog_instance, pkt, need_skip))
+        return;
+
+    rem_after = PACKET_remaining(pkt);
+    QLOG_U64("length", rem_before - rem_after);
+}
+
 static int log_frames(QLOG *qlog_instance,
                       const OSSL_QTX_IOVEC *iovec,
                       size_t num_iovec)
 {
     size_t i;
     PACKET pkt;
-    size_t rem_before, rem_after;
     uint64_t need_skip = 0;
 
     for (i = 0; i < num_iovec; ++i) {
@@ -538,17 +551,10 @@ static int log_frames(QLOG *qlog_instance,
                 continue;
             }
 
-            rem_before = PACKET_remaining(&pkt);
-
             QLOG_BEGIN(NULL)
             {
-                if (!log_frame(qlog_instance, &pkt, &need_skip))
-                    goto cont;
-
-                rem_after = PACKET_remaining(&pkt);
-                QLOG_U64("length", rem_before - rem_after);
+                log_frame(qlog_instance, &pkt, &need_skip);
             }
-cont:
             QLOG_END()
         }
     }
diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index ea6a768185..ff394bbe72 100644
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -1504,43 +1504,41 @@ static int ch_on_transport_params(const unsigned char *params,
             break;
 
         case QUIC_TPARAM_PREFERRED_ADDR:
-            {
-                /* TODO(QUIC FUTURE): Handle preferred address. */
-                if (got_preferred_addr) {
-                    reason = TP_REASON_DUP("PREFERRED_ADDR");
-                    goto malformed;
-                }
+            /* TODO(QUIC FUTURE): Handle preferred address. */
+            if (got_preferred_addr) {
+                reason = TP_REASON_DUP("PREFERRED_ADDR");
+                goto malformed;
+            }
 
-                /*
-                 * RFC 9000 s. 18.2: "A server that chooses a zero-length
-                 * connection ID MUST NOT provide a preferred address.
-                 * Similarly, a server MUST NOT include a zero-length connection
-                 * ID in this transport parameter. A client MUST treat a
-                 * violation of these requirements as a connection error of type
-                 * TRANSPORT_PARAMETER_ERROR."
-                 */
-                if (ch->is_server) {
-                    reason = TP_REASON_SERVER_ONLY("PREFERRED_ADDR");
-                    goto malformed;
-                }
-
-                if (ch->cur_remote_dcid.id_len == 0) {
-                    reason = "PREFERRED_ADDR provided for zero-length CID";
-                    goto malformed;
-                }
-
-                if (!ossl_quic_wire_decode_transport_param_preferred_addr(&pkt, &pfa)) {
-                    reason = TP_REASON_MALFORMED("PREFERRED_ADDR");
-                    goto malformed;
-                }
-
-                if (pfa.cid.id_len == 0) {
-                    reason = "zero-length CID in PREFERRED_ADDR";
-                    goto malformed;
-                }
-
-                got_preferred_addr = 1;
+            /*
+             * RFC 9000 s. 18.2: "A server that chooses a zero-length
+             * connection ID MUST NOT provide a preferred address.
+             * Similarly, a server MUST NOT include a zero-length connection
+             * ID in this transport parameter. A client MUST treat a
+             * violation of these requirements as a connection error of type
+             * TRANSPORT_PARAMETER_ERROR."
+             */
+            if (ch->is_server) {
+                reason = TP_REASON_SERVER_ONLY("PREFERRED_ADDR");
+                goto malformed;
+            }
+
+            if (ch->cur_remote_dcid.id_len == 0) {
+                reason = "PREFERRED_ADDR provided for zero-length CID";
+                goto malformed;
+            }
+
+            if (!ossl_quic_wire_decode_transport_param_preferred_addr(&pkt, &pfa)) {
+                reason = TP_REASON_MALFORMED("PREFERRED_ADDR");
+                goto malformed;
             }
+
+            if (pfa.cid.id_len == 0) {
+                reason = "zero-length CID in PREFERRED_ADDR";
+                goto malformed;
+            }
+
+            got_preferred_addr = 1;
             break;
 
         case QUIC_TPARAM_DISABLE_ACTIVE_MIGRATION: