From c3c99fc1df6ba9cbd02dcd8083b2dfd0c96fa242 Mon Sep 17 00:00:00 2001 From: "Wesierski, Dawid" Date: Thu, 12 Sep 2024 13:10:27 +0200 Subject: [PATCH] Fix: fix and refactor st41 possible errors * Fix possible null pointer reference in app_tx_fmd_init * Fix: coveriy out of band issue * Increate quality of tx_fastmetadata_session_build_packet code * coverity fix: fixing less-than-zero comparison * add st41 context app multithread access error * Add error message when the st41 functionality of RxTx app detects multithread access to the contex app resource, which should not be shared. Fixes: c43152d9 Co-developed-by: Aleksandr Ivanov Co-developed-by: Kolelis, Szymon --- app/src/parse_json.c | 6 ++++-- app/src/tx_fastmetadata_app.c | 22 ++++++++++++++------- lib/src/st2110/st_tx_fastmetadata_session.c | 18 +++++++++++++---- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/app/src/parse_json.c b/app/src/parse_json.c index e9f3708bc..92c8634a3 100644 --- a/app/src/parse_json.c +++ b/app/src/parse_json.c @@ -1255,7 +1255,7 @@ static int st_json_parse_tx_fmd(int idx, json_object* fmd_obj, st_json_object_object_get(fmd_obj, "fastmetadata_data_item_type"); if (fmd_dit_obj) { uint32_t fmd_dit = json_object_get_int(fmd_dit_obj); - if (fmd_dit < 0 || fmd_dit > 0x3fffff) { + if (fmd_dit > 0x3fffff) { err("%s, invalid fastmetadata_data_item_type 0x%x\n", __func__, fmd_dit); return -ST_JSON_NOT_VALID; } @@ -1269,8 +1269,10 @@ static int st_json_parse_tx_fmd(int idx, json_object* fmd_obj, /* parse fmd data item K-bit */ json_object* fmd_k_bit_obj = st_json_object_object_get(fmd_obj, "fastmetadata_k_bit"); if (fmd_k_bit_obj) { + /* assign to uint and check if the value more then 1 + * (the value should be in range of [0,1]) */ uint8_t fmd_k_bit = json_object_get_int(fmd_k_bit_obj); - if (fmd_k_bit < 0 || fmd_k_bit > 1) { + if (fmd_k_bit > 1) { err("%s, invalid fastmetadata_k_bit 0x%x\n", __func__, fmd_k_bit); return -ST_JSON_NOT_VALID; } diff --git a/app/src/tx_fastmetadata_app.c b/app/src/tx_fastmetadata_app.c index b3c422ecf..2b9dab931 100644 --- a/app/src/tx_fastmetadata_app.c +++ b/app/src/tx_fastmetadata_app.c @@ -94,21 +94,29 @@ static void* app_tx_fmd_frame_thread(void* arg) { info("%s(%d), start\n", __func__, idx); while (!s->st41_app_thread_stop) { st_pthread_mutex_lock(&s->st41_wake_mutex); - producer_idx = s->framebuff_producer_idx; - framebuff = &s->framebuffs[producer_idx]; - if (ST_TX_FRAME_FREE != framebuff->stat) { - /* not in free */ - if (!s->st41_app_thread_stop) + framebuff = &s->framebuffs[s->framebuff_producer_idx]; + + if (framebuff->stat != ST_TX_FRAME_FREE) { + if (!s->st41_app_thread_stop) { st_pthread_cond_wait(&s->st41_wake_cond, &s->st41_wake_mutex); + } st_pthread_mutex_unlock(&s->st41_wake_mutex); continue; } + + producer_idx = s->framebuff_producer_idx; st_pthread_mutex_unlock(&s->st41_wake_mutex); struct st41_frame* frame_addr = st41_tx_get_framebuffer(s->handle, producer_idx); app_tx_fmd_build_frame(s, frame_addr); st_pthread_mutex_lock(&s->st41_wake_mutex); + if (producer_idx != s->framebuff_producer_idx) { + err("%s(%d), Apps can't share the context app structure\n", __func__, idx); + st_pthread_mutex_unlock(&s->st41_wake_mutex); + return NULL; + } + framebuff->size = sizeof(*frame_addr); framebuff->stat = ST_TX_FRAME_READY; /* point to next */ @@ -440,8 +448,8 @@ static int app_tx_fmd_init(struct st_app_context* ctx, ops.notify_rtp_done = app_tx_fmd_rtp_done; ops.framebuff_cnt = s->framebuff_cnt; ops.fps = fmd ? fmd->info.fmd_fps : ST_FPS_P59_94; - ops.fmd_dit = fmd->info.fmd_dit; - ops.fmd_k_bit = fmd->info.fmd_k_bit; + ops.fmd_dit = fmd ? fmd->info.fmd_dit : 0; + ops.fmd_k_bit = fmd ? fmd->info.fmd_k_bit : 0; s->st41_pcap_input = false; ops.type = fmd ? fmd->info.type : ST41_TYPE_FRAME_LEVEL; ops.interlaced = fmd ? fmd->info.interlaced : false; diff --git a/lib/src/st2110/st_tx_fastmetadata_session.c b/lib/src/st2110/st_tx_fastmetadata_session.c index adfc49e59..83dc092ef 100644 --- a/lib/src/st2110/st_tx_fastmetadata_session.c +++ b/lib/src/st2110/st_tx_fastmetadata_session.c @@ -389,15 +389,20 @@ static int tx_fastmetadata_session_update_redundant( static void tx_fastmetadata_session_build_packet( struct st_tx_fastmetadata_session_impl* s, struct rte_mbuf* pkt) { - struct mt_udp_hdr* hdr; + struct st41_fmd_hdr* hdr; struct rte_ipv4_hdr* ipv4; struct rte_udp_hdr* udp; struct st41_rtp_hdr* rtp; - hdr = rte_pktmbuf_mtod(pkt, struct mt_udp_hdr*); + if (rte_pktmbuf_data_len(pkt) < sizeof(*hdr)) { + err("%s: packet is less than fmd hdr size", __func__); + return; + } + + hdr = rte_pktmbuf_mtod(pkt, struct st41_fmd_hdr*); ipv4 = &hdr->ipv4; udp = &hdr->udp; - rtp = (struct st41_rtp_hdr*)&udp[1]; + rtp = &hdr->rtp; /* copy the hdr: eth, ip, udp */ rte_memcpy(&hdr->eth, &s->hdr[MTL_SESSION_PORT_P].eth, sizeof(hdr->eth)); @@ -417,7 +422,6 @@ static void tx_fastmetadata_session_build_packet( rtp->base.tmstamp = htonl(s->pacing.rtp_time_stamp); /* Set place for payload just behind rtp header */ - uint8_t* payload = (uint8_t*)&rtp[1]; struct st_frame_trans* frame_info = &s->st41_frames[s->st41_frame_idx]; uint32_t offset = s->st41_pkt_idx * s->max_pkt_len; void* src_addr = frame_info->addr + offset; @@ -426,6 +430,12 @@ static void tx_fastmetadata_session_build_packet( uint16_t data_item_length = (data_item_length_bytes + 3) / 4; /* expressed in number of 4-byte words */ + if (rte_pktmbuf_data_len(pkt) < sizeof(*hdr) + data_item_length) { + err("%s: packet doesn't contain RTP payload", __func__); + return; + } + + uint8_t* payload = (uint8_t*)(rtp + 1); if (!(data_item_length_bytes > s->max_pkt_len)) { int offset = 0; for (int i = 0; i < data_item_length_bytes; i++) {