From a043c3621976126dfaaaf8ffa2ee7fd5d98ea33b Mon Sep 17 00:00:00 2001 From: Francis Deslauriers Date: Wed, 10 Nov 2021 13:16:15 -0500 Subject: [PATCH] Fix: src.ctf.lttng-live: using `last_inactivity_ts` uninitialized Observed issue ============== I saw the following error message in the logs of a `lttng clear` test case crash: bt_clock_class_cycles_to_ns_from_origin@clock-class.c:311 Cannot convert cycles to nanoseconds from origin for given clock class: value overflows the signed 64-bit integer range: cc-addr=0x55f5b33280f0, cc-name="monotonic", cc-freq=1000000000, cc-partial-descr="Monotonic Clock", cc-uuid="c812f5e1-ceda-4f13-8b99-23a5be8a2908", cc-is-frozen=0, cc-precision=0, cc-offset-s=1634653310, cc-offset-cycles=492621904, cc-origin-is-unix-epoch=1, cc-base-offset-ns=1634653310492621904, cc-cs-pool-size=6, cc-cs-pool-cap=15, cycles=9223372036854775808 Take note that last line of this log statement shows a `cycles` value of 9223372036854775808. This value is `INT64_MIN` stored on a 64bits unsigned integer. Cause ===== This error occurs because the `lttng_live_stream_iterator::last_inactivity_ts` field is used without being initialized to an actual value. This field is a `uint64_t` and is initialized to `INT64_MIN` with the intent of setting it to the minimum value and used as is in that error statement. Fix === Change the `last_inactivity_ts` field to an anonymous struct that contains an `is_set` boolean and the value. Use the `is_set` flag to ensure we only use this field once initialized. Signed-off-by: Francis Deslauriers Change-Id: I24ba0dd9267508b2c9f0de99386a16b1ce022329 Reviewed-on: https://review.lttng.org/c/babeltrace/+/6677 Tested-by: jenkins Reviewed-by: Philippe Proulx --- src/plugins/ctf/lttng-live/data-stream.c | 3 ++- src/plugins/ctf/lttng-live/lttng-live.c | 31 ++++++++++++++++-------- src/plugins/ctf/lttng-live/lttng-live.h | 5 +++- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/plugins/ctf/lttng-live/data-stream.c b/src/plugins/ctf/lttng-live/data-stream.c index 618af36d..d7fdf346 100644 --- a/src/plugins/ctf/lttng-live/data-stream.c +++ b/src/plugins/ctf/lttng-live/data-stream.c @@ -223,7 +223,8 @@ struct lttng_live_stream_iterator *lttng_live_stream_iterator_create( stream_iter->state = LTTNG_LIVE_STREAM_ACTIVE_NO_DATA; stream_iter->viewer_stream_id = stream_id; stream_iter->ctf_stream_class_id = -1ULL; - stream_iter->last_inactivity_ts = INT64_MIN; + stream_iter->last_inactivity_ts.is_set = false; + stream_iter->last_inactivity_ts.value = 0; if (trace->trace) { struct ctf_trace_class *ctf_tc = diff --git a/src/plugins/ctf/lttng-live/lttng-live.c b/src/plugins/ctf/lttng-live/lttng-live.c index b0833061..2eafb2a6 100644 --- a/src/plugins/ctf/lttng-live/lttng-live.c +++ b/src/plugins/ctf/lttng-live/lttng-live.c @@ -103,10 +103,12 @@ void lttng_live_stream_iterator_set_state(struct lttng_live_stream_iterator *str #define LTTNG_LIVE_LOGD_STREAM_ITER(live_stream_iter) \ do { \ - BT_COMP_LOGD("Live stream iterator state=%s, last-inact-ts=%" PRId64 \ - ", curr-inact-ts %" PRId64, \ + BT_COMP_LOGD("Live stream iterator state=%s, " \ + "last-inact-ts-is-set=%d, last-inact-ts-value=%" PRId64 "), " \ + "curr-inact-ts %" PRId64, \ lttng_live_stream_state_string(live_stream_iter->state), \ - live_stream_iter->last_inactivity_ts, \ + live_stream_iter->last_inactivity_ts.is_set, \ + live_stream_iter->last_inactivity_ts.value, \ live_stream_iter->current_inactivity_ts); \ } while (0); @@ -418,11 +420,18 @@ enum lttng_live_iterator_status lttng_live_iterator_next_handle_one_no_data_stre BT_ASSERT_DBG(lttng_live_stream->state != LTTNG_LIVE_STREAM_EOF); if (lttng_live_stream->state == LTTNG_LIVE_STREAM_QUIESCENT) { - uint64_t last_inact_ts = lttng_live_stream->last_inactivity_ts, + uint64_t last_inact_ts = lttng_live_stream->last_inactivity_ts.value, curr_inact_ts = lttng_live_stream->current_inactivity_ts; if (orig_state == LTTNG_LIVE_STREAM_QUIESCENT_NO_DATA && last_inact_ts == curr_inact_ts) { + /* + * Because the stream is in the QUIESCENT_NO_DATA + * state, we can assert that the last_inactivity_ts was + * set and can be safely used in the `if` above. + */ + BT_ASSERT(lttng_live_stream->last_inactivity_ts.is_set); + ret = LTTNG_LIVE_ITERATOR_STATUS_AGAIN; LTTNG_LIVE_LOGD_STREAM_ITER(lttng_live_stream); } else { @@ -700,8 +709,9 @@ enum lttng_live_iterator_status lttng_live_iterator_next_handle_one_quiescent_st * Check if we already sent an inactivty message downstream for this * `current_inactivity_ts` value. */ - if (lttng_live_stream->current_inactivity_ts == - lttng_live_stream->last_inactivity_ts) { + if (lttng_live_stream->last_inactivity_ts.is_set && + lttng_live_stream->current_inactivity_ts == + lttng_live_stream->last_inactivity_ts.value) { lttng_live_stream_iterator_set_state(lttng_live_stream, LTTNG_LIVE_STREAM_QUIESCENT_NO_DATA); @@ -712,8 +722,9 @@ enum lttng_live_iterator_status lttng_live_iterator_next_handle_one_quiescent_st ret = emit_inactivity_message(lttng_live_msg_iter, lttng_live_stream, message, lttng_live_stream->current_inactivity_ts); - lttng_live_stream->last_inactivity_ts = + lttng_live_stream->last_inactivity_ts.value = lttng_live_stream->current_inactivity_ts; + lttng_live_stream->last_inactivity_ts.is_set = true; end: return ret; } @@ -1152,7 +1163,7 @@ enum lttng_live_iterator_status handle_late_message( clock_class = bt_stream_class_borrow_default_clock_class_const(stream_class); ts_ns_status = bt_clock_class_cycles_to_ns_from_origin(clock_class, - stream_iter->last_inactivity_ts, &last_inactivity_ts_ns); + stream_iter->last_inactivity_ts.value, &last_inactivity_ts_ns); if (ts_ns_status != BT_CLOCK_CLASS_CYCLES_TO_NS_FROM_ORIGIN_STATUS_OK) { stream_iter_status = LTTNG_LIVE_ITERATOR_STATUS_ERROR; goto end; @@ -1185,14 +1196,14 @@ enum lttng_live_iterator_status handle_late_message( lttng_live_msg_iter->self_msg_iter, stream_iter->stream, late_msg, &adjusted_message, - stream_iter->last_inactivity_ts); + stream_iter->last_inactivity_ts.value); break; case BT_MESSAGE_TYPE_DISCARDED_PACKETS: adjust_status = adjust_discarded_packets_message( lttng_live_msg_iter->self_msg_iter, stream_iter->stream, late_msg, &adjusted_message, - stream_iter->last_inactivity_ts); + stream_iter->last_inactivity_ts.value); break; default: bt_common_abort(); diff --git a/src/plugins/ctf/lttng-live/lttng-live.h b/src/plugins/ctf/lttng-live/lttng-live.h index 82f39a0f..be235a7d 100644 --- a/src/plugins/ctf/lttng-live/lttng-live.h +++ b/src/plugins/ctf/lttng-live/lttng-live.h @@ -78,7 +78,10 @@ struct lttng_live_stream_iterator { * Clock Snapshot value of the last message iterator inactivity message * sent downstream. */ - uint64_t last_inactivity_ts; + struct { + bool is_set; + uint64_t value; + } last_inactivity_ts; /* * Clock Snapshot value of the current message iterator inactivity -- 2.34.1