From 6a8eea3c94a83673bcbd621d5d928dfb071c1b61 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 5 Sep 2022 12:51:17 +0200 Subject: [PATCH 01/14] add stream list interface --- include/srtp_priv.h | 1 + include/stream_list_priv.h | 118 +++++++++++++++++++++++++++++++++++++ srtp/srtp.c | 92 +++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+) create mode 100644 include/stream_list_priv.h diff --git a/include/srtp_priv.h b/include/srtp_priv.h index 48dc65c7d..ade8622e8 100644 --- a/include/srtp_priv.h +++ b/include/srtp_priv.h @@ -66,6 +66,7 @@ extern "C" { typedef struct srtp_stream_ctx_t_ srtp_stream_ctx_t; typedef srtp_stream_ctx_t *srtp_stream_t; +typedef struct srtp_stream_list_ctx_t_ *srtp_stream_list_t; /* * the following declarations are libSRTP internal functions diff --git a/include/stream_list_priv.h b/include/stream_list_priv.h new file mode 100644 index 000000000..815d77dce --- /dev/null +++ b/include/stream_list_priv.h @@ -0,0 +1,118 @@ +/* + * stream_list_priv.h + * + * list of SRTP streams, keyed by SSRC + * + * Alba Mendez + */ +/* + * + * Copyright (c) 2001-2017, Cisco Systems, Inc. + * Copyright (c) 2022, Dolby Laboratories, Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * Neither the name of the Cisco Systems, Inc. nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED + * OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#ifndef SRTP_STREAM_LIST_PRIV_H +#define SRTP_STREAM_LIST_PRIV_H + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * srtp_stream_list_t holds a list of srtp_stream_t, each identified + * by their SSRC. + * + * the API was extracted to allow downstreams to override its + * implementation by defining the `SRTP_NO_STREAM_LIST` preprocessor + * directive, which removes the default implementation of these + * functions. if this is done, the `next` field is free for the + * implementation to use. + * + * this is still an internal interface; there is no stability + * guarantee--downstreams should watch this file for changes in + * signatures or semantics. + */ + +/** + * allocate and initialize a stream list instance + */ +srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list); + +/** + * insert a stream into the list + * + * ownership is transferred from caller to the list. + * + * if another stream with the same SSRC already exists in the list, + * behavior is undefined. if the SSRC field is mutated while the + * stream is inserted, further operations have undefined behavior + */ +void srtp_stream_list_insert(srtp_stream_list_t *list, srtp_stream_t stream); + +/* + * look up the stream corresponding to the specified SSRC and return it. + * if no such SSRC is found, NULL is returned. + */ +srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list, uint32_t ssrc); + +/** + * delete the stream associated to the specified SSRC. + * + * if a stream is found and removed, it's returned and ownership is + * transferred to the caller. if not found, NULL is returned. + */ +srtp_stream_t srtp_stream_list_delete(srtp_stream_list_t *list, uint32_t ssrc); + +/** + * iterate through all stored streams. while iterating, it is allowed to delete + * the current element; any other mutation to the list is undefined behavior. + * returning non-zero from callback aborts the iteration. + */ +void srtp_stream_list_for_each(srtp_stream_list_t *list, + int (*callback)(srtp_stream_t, void *), + void *data); + +/** + * deallocate a stream list instance and all streams inserted in it + */ +srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t *list, + srtp_stream_t template_); + +#ifdef __cplusplus +} +#endif + +#endif /* SRTP_STREAM_LIST_PRIV_H */ diff --git a/srtp/srtp.c b/srtp/srtp.c index 2d3faaf47..af1db9858 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -46,6 +46,7 @@ #include "config.h" #include "srtp_priv.h" +#include "stream_list_priv.h" #include "crypto_types.h" #include "err.h" #include "alloc.h" /* for srtp_crypto_alloc() */ @@ -4747,3 +4748,94 @@ srtp_err_status_t srtp_get_stream_roc(srtp_t session, return srtp_err_status_ok; } + +#ifndef SRTP_NO_STREAM_LIST + +/* in the default implementation, we have an intrusive linked list */ +struct { + srtp_stream_ctx_t dummy; +} srtp_stream_list_ctx_t_; + +srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list) +{ + (*list) = NULL; + return srtp_err_status_ok; +} + +void srtp_stream_list_insert(srtp_stream_list_t *list_, srtp_stream_t stream) +{ + srtp_stream_t *list = (srtp_stream_t *)list_; + + /* insert at the head of the list */ + stream->next = (*list); + (*list) = stream; +} + +srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list_, uint32_t ssrc) +{ + srtp_stream_t *list = (srtp_stream_t *)list_; + + /* walk down list until ssrc is found */ + srtp_stream_t stream = (*list); + while (stream != NULL) { + if (stream->ssrc == ssrc) + return stream; + stream = stream->next; + } + + /* we haven't found our ssrc, so return a null */ + return NULL; +} + +srtp_stream_t srtp_stream_list_delete(srtp_stream_list_t *list_, uint32_t ssrc) +{ + srtp_stream_t *list = (srtp_stream_t *)list_; + + /* walk down list until ssrc is found */ + srtp_stream_t *stream = list; + while ((*stream) != NULL) { + if ((*stream)->ssrc == ssrc) { + srtp_stream_t tmp = (*stream); + (*stream) = tmp->next; + return tmp; + } + stream = &(*stream)->next; + } + + /* we haven't found our ssrc, so return a null */ + return NULL; +} + +void srtp_stream_list_for_each(srtp_stream_list_t *list_, + int (*callback)(srtp_stream_t, void *), + void *data) +{ + srtp_stream_t *list = (srtp_stream_t *)list_; + srtp_stream_t stream = (*list); + while (stream != NULL) { + srtp_stream_t tmp = stream; + stream = stream->next; + if (callback(tmp, data)) + break; + } +} + +srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t *list_, + srtp_stream_t template) +{ + srtp_stream_t *list = (srtp_stream_t *)list_; + srtp_err_status_t status; + + /* walk list of streams, deallocating as we go */ + while ((*list) != NULL) { + srtp_stream_t next = (*list)->next; + status = srtp_stream_dealloc((*list), template); + if (status) + return status; + (*list) = next; + } + + return srtp_err_status_ok; +} + +#endif From b8734bb7443ceb63374b44fd01d78fb92572e69a Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Tue, 6 Sep 2022 02:32:29 +0200 Subject: [PATCH 02/14] 1/2 use it --- srtp/srtp.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index af1db9858..940bc4dd6 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -2056,9 +2056,8 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx, return status; } - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set stream (the pointer used in this function) */ stream = new_stream; @@ -2147,9 +2146,8 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx, if (status) return status; - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set direction to outbound */ new_stream->direction = dir_srtp_sender; @@ -2746,9 +2744,8 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx, if (status) return status; - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set stream (the pointer used in this function) */ stream = new_stream; @@ -3900,9 +3897,8 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead( return status; } - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set stream (the pointer used in this function) */ stream = new_stream; @@ -3966,9 +3962,8 @@ srtp_err_status_t srtp_protect_rtcp_mki(srtp_t ctx, if (status) return status; - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4413,9 +4408,8 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx, if (status) return status; - /* add new stream to the head of the stream_list */ - new_stream->next = ctx->stream_list; - ctx->stream_list = new_stream; + /* add new stream to the list */ + srtp_stream_list_insert(&ctx->stream_list, new_stream); /* set stream (the pointer used in this function) */ stream = new_stream; From 8e314717511d9e410e0454139ee8334722a25bc8 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 5 Sep 2022 23:33:04 +0200 Subject: [PATCH 03/14] 2/2 use it --- include/srtp_priv.h | 2 +- srtp/srtp.c | 71 +++++++++++++++------------------------------ 2 files changed, 24 insertions(+), 49 deletions(-) diff --git a/include/srtp_priv.h b/include/srtp_priv.h index ade8622e8..f1977e2e9 100644 --- a/include/srtp_priv.h +++ b/include/srtp_priv.h @@ -157,7 +157,7 @@ typedef struct srtp_stream_ctx_t_ { * an srtp_ctx_t holds a stream list and a service description */ typedef struct srtp_ctx_t_ { - struct srtp_stream_ctx_t_ *stream_list; /* linked list of streams */ + srtp_stream_list_t stream_list; /* linked list of streams */ struct srtp_stream_ctx_t_ *stream_template; /* act as template for other */ /* streams */ void *user_data; /* user custom data */ diff --git a/srtp/srtp.c b/srtp/srtp.c index 940bc4dd6..0ffeff8d1 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -2803,32 +2803,13 @@ srtp_err_status_t srtp_shutdown() return srtp_err_status_ok; } -/* - * srtp_get_stream(ssrc) returns a pointer to the stream corresponding - * to ssrc, or NULL if no stream exists for that ssrc - * - * this is an internal function - */ - srtp_stream_ctx_t *srtp_get_stream(srtp_t srtp, uint32_t ssrc) { - srtp_stream_ctx_t *stream; - - /* walk down list until ssrc is found */ - stream = srtp->stream_list; - while (stream != NULL) { - if (stream->ssrc == ssrc) - return stream; - stream = stream->next; - } - - /* we haven't found our ssrc, so return a null */ - return NULL; + return srtp_stream_list_get(&srtp->stream_list, ssrc); } srtp_err_status_t srtp_dealloc(srtp_t session) { - srtp_stream_ctx_t *stream; srtp_err_status_t status; /* @@ -2837,15 +2818,11 @@ srtp_err_status_t srtp_dealloc(srtp_t session) * memory and just return an error */ - /* walk list of streams, deallocating as we go */ - stream = session->stream_list; - while (stream != NULL) { - srtp_stream_t next = stream->next; - status = srtp_stream_dealloc(stream, session->stream_template); - if (status) - return status; - stream = next; - } + /* deallocate streams */ + status = srtp_stream_list_dealloc(&session->stream_list, + session->stream_template); + if (status) + return status; /* deallocate stream template, if there is one */ if (session->stream_template != NULL) { @@ -2914,8 +2891,7 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) session->stream_template->direction = dir_srtp_receiver; break; case (ssrc_specific): - tmp->next = session->stream_list; - session->stream_list = tmp; + srtp_stream_list_insert(&session->stream_list, tmp); break; case (ssrc_undefined): default: @@ -2947,13 +2923,23 @@ srtp_err_status_t srtp_create(srtp_t *session, /* handle for session */ return srtp_err_status_alloc_fail; *session = ctx; + ctx->stream_template = NULL; + ctx->stream_list = NULL; + ctx->user_data = NULL; + + /* allocate stream list */ + stat = srtp_stream_list_create(&ctx->stream_list); + if (stat) { + /* clean up everything */ + srtp_dealloc(*session); + *session = NULL; + return stat; + } + /* * loop over elements in the policy list, allocating and * initializing a stream for each element */ - ctx->stream_template = NULL; - ctx->stream_list = NULL; - ctx->user_data = NULL; while (policy != NULL) { stat = srtp_add_stream(ctx, policy); if (stat) { @@ -2972,29 +2958,18 @@ srtp_err_status_t srtp_create(srtp_t *session, /* handle for session */ srtp_err_status_t srtp_remove_stream(srtp_t session, uint32_t ssrc) { - srtp_stream_ctx_t *stream, *last_stream; + srtp_stream_ctx_t *stream; srtp_err_status_t status; /* sanity check arguments */ if (session == NULL) return srtp_err_status_bad_param; - /* find stream in list; complain if not found */ - last_stream = stream = session->stream_list; - while ((stream != NULL) && (ssrc != stream->ssrc)) { - last_stream = stream; - stream = stream->next; - } + /* find and remove stream from the list */ + stream = srtp_stream_list_delete(&session->stream_list, ssrc); if (stream == NULL) return srtp_err_status_no_ctx; - /* remove stream from the list */ - if (last_stream == stream) - /* stream was first in list */ - session->stream_list = stream->next; - else - last_stream->next = stream->next; - /* deallocate the stream */ status = srtp_stream_dealloc(stream, session->stream_template); if (status) From feac4cea58d6d88a49c7a07d7690c69f2562b283 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 5 Sep 2022 23:35:49 +0200 Subject: [PATCH 04/14] use in update_template_streams() to make it work with the generalized interface, instead of removing only the matching streams and then merging both lists, we remove & add every stream. the old stream is then deallocated and replaced by the newly produced one --- srtp/srtp.c | 140 +++++++++++++++++++++++++++------------------------- 1 file changed, 72 insertions(+), 68 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index 0ffeff8d1..16586175d 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -3005,12 +3005,60 @@ srtp_err_status_t srtp_update(srtp_t session, const srtp_policy_t *policy) return srtp_err_status_ok; } +struct update_template_stream_data { + srtp_err_status_t status; + srtp_t session; + srtp_stream_t new_stream_template; + srtp_stream_list_t *new_stream_list; +}; + +static int update_template_stream(srtp_stream_t stream, void *raw_data) +{ + struct update_template_stream_data *data = + (struct update_template_stream_data *)raw_data; + srtp_t session = data->session; + uint32_t ssrc = stream->ssrc; + srtp_xtd_seq_num_t old_index; + srtp_rdb_t old_rtcp_rdb; + + /* old / non-template streams are copied unchanged */ + if (stream->session_keys[0].rtp_auth != + session->stream_template->session_keys[0].rtp_auth) { + srtp_stream_list_delete(&session->stream_list, ssrc); + srtp_stream_list_insert(data->new_stream_list, stream); + return 0; + } + + /* save old extendard seq */ + old_index = stream->rtp_rdbx.index; + old_rtcp_rdb = stream->rtcp_rdb; + + /* remove stream */ + data->status = srtp_remove_stream(session, ssrc); + if (data->status) + return 1; + + /* allocate and initialize a new stream */ + data->status = srtp_stream_clone(data->new_stream_template, ssrc, &stream); + if (data->status) + return 1; + + /* add new stream to the head of the new_stream_list */ + srtp_stream_list_insert(data->new_stream_list, stream); + + /* restore old extended seq */ + stream->rtp_rdbx.index = old_index; + stream->rtcp_rdb = old_rtcp_rdb; + + return 0; +} + static srtp_err_status_t update_template_streams(srtp_t session, const srtp_policy_t *policy) { srtp_err_status_t status; srtp_stream_t new_stream_template; - srtp_stream_t new_stream_list = NULL; + srtp_stream_list_t new_stream_list; status = srtp_valid_policy(policy); if (status != srtp_err_status_ok) { @@ -3034,77 +3082,33 @@ static srtp_err_status_t update_template_streams(srtp_t session, return status; } - /* for all old templated streams */ - for (;;) { - srtp_stream_t stream; - uint32_t ssrc; - srtp_xtd_seq_num_t old_index; - srtp_rdb_t old_rtcp_rdb; - - stream = session->stream_list; - while ((stream != NULL) && - (stream->session_keys[0].rtp_auth != - session->stream_template->session_keys[0].rtp_auth)) { - stream = stream->next; - } - if (stream == NULL) { - /* no more templated streams */ - break; - } - - /* save old extendard seq */ - ssrc = stream->ssrc; - old_index = stream->rtp_rdbx.index; - old_rtcp_rdb = stream->rtcp_rdb; - - /* remove stream */ - status = srtp_remove_stream(session, ssrc); - if (status) { - /* free new allocations */ - while (new_stream_list != NULL) { - srtp_stream_t next = new_stream_list->next; - srtp_stream_dealloc(new_stream_list, new_stream_template); - new_stream_list = next; - } - srtp_stream_dealloc(new_stream_template, NULL); - return status; - } - - /* allocate and initialize a new stream */ - status = srtp_stream_clone(new_stream_template, ssrc, &stream); - if (status) { - /* free new allocations */ - while (new_stream_list != NULL) { - srtp_stream_t next = new_stream_list->next; - srtp_stream_dealloc(new_stream_list, new_stream_template); - new_stream_list = next; - } - srtp_stream_dealloc(new_stream_template, NULL); - return status; - } - - /* add new stream to the head of the new_stream_list */ - stream->next = new_stream_list; - new_stream_list = stream; + /* allocate new stream list */ + status = srtp_stream_list_create(&new_stream_list); + if (status) { + srtp_crypto_free(new_stream_template); + return status; + } - /* restore old extended seq */ - stream->rtp_rdbx.index = old_index; - stream->rtcp_rdb = old_rtcp_rdb; + /* process streams */ + struct update_template_stream_data data = { srtp_err_status_ok, session, + new_stream_template, + &new_stream_list }; + srtp_stream_list_for_each(&session->stream_list, &update_template_stream, + &data); + if (data.status) { + /* free new allocations */ + srtp_stream_list_dealloc(&new_stream_list, new_stream_template); + srtp_stream_dealloc(new_stream_template, NULL); + return data.status; } - /* dealloc old template */ + + /* dealloc old list / template */ + srtp_stream_list_dealloc(&session->stream_list, session->stream_template); srtp_stream_dealloc(session->stream_template, NULL); - /* set new template */ + /* set new list / template */ session->stream_template = new_stream_template; - /* add new list */ - if (new_stream_list) { - srtp_stream_t tail = new_stream_list; - while (tail->next) { - tail = tail->next; - } - tail->next = session->stream_list; - session->stream_list = new_stream_list; - } - return status; + session->stream_list = new_stream_list; + return srtp_err_status_ok; } static srtp_err_status_t update_stream(srtp_t session, From 6653a8667e9065ddbd5f8f914324121aa22a7488 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Tue, 6 Sep 2022 02:34:07 +0200 Subject: [PATCH 05/14] use in get_protect_trailer_length --- srtp/srtp.c | 53 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index 16586175d..3ba259059 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -4569,6 +4569,32 @@ srtp_err_status_t stream_get_protect_trailer_length(srtp_stream_ctx_t *stream, return srtp_err_status_ok; } +struct get_protect_trailer_length_data { + int valid; + uint32_t length; + uint32_t is_rtp; + uint32_t use_mki; + uint32_t mki_index; +}; + +static int get_protect_trailer_length_cb(srtp_stream_t stream, void *raw_data) +{ + struct get_protect_trailer_length_data *data = + (struct get_protect_trailer_length_data *)raw_data; + uint32_t temp_length; + + data->valid = 1; + if (stream_get_protect_trailer_length(stream, data->is_rtp, data->use_mki, + data->mki_index, + &temp_length) == srtp_err_status_ok) { + if (temp_length > data->length) { + data->length = temp_length; + } + } + + return 0; +} + srtp_err_status_t get_protect_trailer_length(srtp_t session, uint32_t is_rtp, uint32_t use_mki, @@ -4576,38 +4602,29 @@ srtp_err_status_t get_protect_trailer_length(srtp_t session, uint32_t *length) { srtp_stream_ctx_t *stream; + struct get_protect_trailer_length_data data = { 0, 0, is_rtp, use_mki, + mki_index }; if (session == NULL) { return srtp_err_status_bad_param; } - if (session->stream_template == NULL && session->stream_list == NULL) { - return srtp_err_status_bad_param; - } - - *length = 0; - stream = session->stream_template; if (stream != NULL) { + data.valid = 1; stream_get_protect_trailer_length(stream, is_rtp, use_mki, mki_index, - length); + &data.length); } - stream = session->stream_list; + srtp_stream_list_for_each(&session->stream_list, + &get_protect_trailer_length_cb, &data); - while (stream != NULL) { - uint32_t temp_length; - if (stream_get_protect_trailer_length(stream, is_rtp, use_mki, - mki_index, &temp_length) == - srtp_err_status_ok) { - if (temp_length > *length) { - *length = temp_length; - } - } - stream = stream->next; + if (!data.valid) { + return srtp_err_status_bad_param; } + *length = data.length; return srtp_err_status_ok; } From f161ac16c28c9552729cfe3850cd6886b57f96d5 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 5 Sep 2022 12:57:10 +0200 Subject: [PATCH 06/14] expose missing internal method custom implementations need to have it available --- include/srtp_priv.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/srtp_priv.h b/include/srtp_priv.h index f1977e2e9..90045942d 100644 --- a/include/srtp_priv.h +++ b/include/srtp_priv.h @@ -103,6 +103,9 @@ srtp_err_status_t srtp_steam_init_all_master_keys( */ srtp_err_status_t srtp_stream_init(srtp_stream_t srtp, const srtp_policy_t *p); +srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, + const srtp_stream_ctx_t *stream_template); + /* * libsrtp internal datatypes */ From 6b0aa2dadbc41dd1f34e20324b7d86e6aff57faa Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Wed, 7 Sep 2022 02:42:46 +0200 Subject: [PATCH 07/14] update tests removes code duplication as well --- test/srtp_driver.c | 155 ++++++++++++++++++++++----------------------- 1 file changed, 74 insertions(+), 81 deletions(-) diff --git a/test/srtp_driver.c b/test/srtp_driver.c index fd199c5bf..1ddb5cd05 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -49,6 +49,7 @@ #include "getopt_s.h" /* for local getopt() */ #include "srtp_priv.h" +#include "stream_list_priv.h" #include "util.h" #ifdef HAVE_NETINET_IN_H @@ -1467,13 +1468,75 @@ srtp_err_status_t srtcp_test(const srtp_policy_t *policy, int mki_index) return srtp_err_status_ok; } +struct srtp_session_print_stream_data { + // set by callback to indicate failure + srtp_err_status_t status; + // indicates if it is the template stream or a regular stream + int is_template; +}; + +int srtp_session_print_stream(srtp_stream_t stream, void *raw_data) +{ + static const char *serv_descr[4] = { "none", "confidentiality", + "authentication", + "confidentiality and authentication" }; + static const char *direction[3] = { "unknown", "outbound", "inbound" }; + + struct srtp_session_print_stream_data *data = + (struct srtp_session_print_stream_data *)raw_data; + srtp_session_keys_t *session_keys = &stream->session_keys[0]; + char ssrc_text[32]; + + if (!data->is_template && stream->rtp_services > sec_serv_conf_and_auth) { + data->status = srtp_err_status_bad_param; + return 1; + } + + if (data->is_template) { + snprintf(ssrc_text, sizeof(ssrc_text), "any %s", + direction[stream->direction]); + } else { + snprintf(ssrc_text, sizeof(ssrc_text), "0x%08x", stream->ssrc); + } + + printf("# SSRC: %s\r\n" + "# rtp cipher: %s\r\n" + "# rtp auth: %s\r\n" + "# rtp services: %s\r\n" + "# rtcp cipher: %s\r\n" + "# rtcp auth: %s\r\n" + "# rtcp services: %s\r\n" + "# window size: %lu\r\n" + "# tx rtx allowed:%s\r\n", + ssrc_text, session_keys->rtp_cipher->type->description, + session_keys->rtp_auth->type->description, + serv_descr[stream->rtp_services], + session_keys->rtcp_cipher->type->description, + session_keys->rtcp_auth->type->description, + serv_descr[stream->rtcp_services], + srtp_rdbx_get_window_size(&stream->rtp_rdbx), + stream->allow_repeat_tx ? "true" : "false"); + + printf("# Encrypted extension headers: "); + if (stream->enc_xtn_hdr && stream->enc_xtn_hdr_count > 0) { + int *enc_xtn_hdr = stream->enc_xtn_hdr; + int count = stream->enc_xtn_hdr_count; + while (count > 0) { + printf("%d ", *enc_xtn_hdr); + enc_xtn_hdr++; + count--; + } + printf("\n"); + } else { + printf("none\n"); + } + + return 0; +} + srtp_err_status_t srtp_session_print_policy(srtp_t srtp) { - char *serv_descr[4] = { "none", "confidentiality", "authentication", - "confidentiality and authentication" }; - char *direction[3] = { "unknown", "outbound", "inbound" }; - srtp_stream_t stream; - srtp_session_keys_t *session_keys = NULL; + struct srtp_session_print_stream_data data = { srtp_err_status_ok, 0 }; /* sanity checking */ if (srtp == NULL) { @@ -1482,86 +1545,16 @@ srtp_err_status_t srtp_session_print_policy(srtp_t srtp) /* if there's a template stream, print it out */ if (srtp->stream_template != NULL) { - stream = srtp->stream_template; - session_keys = &stream->session_keys[0]; - printf("# SSRC: any %s\r\n" - "# rtp cipher: %s\r\n" - "# rtp auth: %s\r\n" - "# rtp services: %s\r\n" - "# rtcp cipher: %s\r\n" - "# rtcp auth: %s\r\n" - "# rtcp services: %s\r\n" - "# window size: %lu\r\n" - "# tx rtx allowed:%s\r\n", - direction[stream->direction], - session_keys->rtp_cipher->type->description, - session_keys->rtp_auth->type->description, - serv_descr[stream->rtp_services], - session_keys->rtcp_cipher->type->description, - session_keys->rtcp_auth->type->description, - serv_descr[stream->rtcp_services], - srtp_rdbx_get_window_size(&stream->rtp_rdbx), - stream->allow_repeat_tx ? "true" : "false"); - - printf("# Encrypted extension headers: "); - if (stream->enc_xtn_hdr && stream->enc_xtn_hdr_count > 0) { - int *enc_xtn_hdr = stream->enc_xtn_hdr; - int count = stream->enc_xtn_hdr_count; - while (count > 0) { - printf("%d ", *enc_xtn_hdr); - enc_xtn_hdr++; - count--; - } - printf("\n"); - } else { - printf("none\n"); - } + data.is_template = 1; + srtp_session_print_stream(srtp->stream_template, &data); } /* loop over streams in session, printing the policy of each */ - stream = srtp->stream_list; - while (stream != NULL) { - if (stream->rtp_services > sec_serv_conf_and_auth) { - return srtp_err_status_bad_param; - } - session_keys = &stream->session_keys[0]; - - printf("# SSRC: 0x%08x\r\n" - "# rtp cipher: %s\r\n" - "# rtp auth: %s\r\n" - "# rtp services: %s\r\n" - "# rtcp cipher: %s\r\n" - "# rtcp auth: %s\r\n" - "# rtcp services: %s\r\n" - "# window size: %lu\r\n" - "# tx rtx allowed:%s\r\n", - stream->ssrc, session_keys->rtp_cipher->type->description, - session_keys->rtp_auth->type->description, - serv_descr[stream->rtp_services], - session_keys->rtcp_cipher->type->description, - session_keys->rtcp_auth->type->description, - serv_descr[stream->rtcp_services], - srtp_rdbx_get_window_size(&stream->rtp_rdbx), - stream->allow_repeat_tx ? "true" : "false"); - - printf("# Encrypted extension headers: "); - if (stream->enc_xtn_hdr && stream->enc_xtn_hdr_count > 0) { - int *enc_xtn_hdr = stream->enc_xtn_hdr; - int count = stream->enc_xtn_hdr_count; - while (count > 0) { - printf("%d ", *enc_xtn_hdr); - enc_xtn_hdr++; - count--; - } - printf("\n"); - } else { - printf("none\n"); - } + data.is_template = 0; + srtp_stream_list_for_each(&srtp->stream_list, &srtp_session_print_stream, + &data); - /* advance to next stream in the list */ - stream = stream->next; - } - return srtp_err_status_ok; + return data.status; } srtp_err_status_t srtp_print_policy(const srtp_policy_t *policy) From 55f409f6129196e0f22c2b28fabce130712b9b09 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 12 Sep 2022 13:31:37 +0200 Subject: [PATCH 08/14] allow insertions to fail --- include/stream_list_priv.h | 7 +++-- srtp/srtp.c | 52 ++++++++++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/include/stream_list_priv.h b/include/stream_list_priv.h index 815d77dce..0245126a4 100644 --- a/include/stream_list_priv.h +++ b/include/stream_list_priv.h @@ -74,13 +74,16 @@ srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list); /** * insert a stream into the list * - * ownership is transferred from caller to the list. + * returns srtp_err_status_alloc_fail if insertion failed due to unavailable + * capacity in the list. if operation succeeds, srtp_err_status_ok is returned + * and ownership is transferred from caller into the list * * if another stream with the same SSRC already exists in the list, * behavior is undefined. if the SSRC field is mutated while the * stream is inserted, further operations have undefined behavior */ -void srtp_stream_list_insert(srtp_stream_list_t *list, srtp_stream_t stream); +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list, + srtp_stream_t stream); /* * look up the stream corresponding to the specified SSRC and return it. diff --git a/srtp/srtp.c b/srtp/srtp.c index 3ba259059..6e65f5348 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -279,6 +279,16 @@ srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, return srtp_err_status_ok; } +/* try to insert stream in list or deallocate it */ +static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t *list, srtp_stream_t stream, srtp_stream_t template) +{ + srtp_err_status_t status = srtp_stream_list_insert(list, stream); + /* on failure, ownership wasn't transferred and we need to deallocate */ + if (status) + srtp_stream_dealloc(stream, template); + return status; +} + static srtp_err_status_t srtp_valid_policy(const srtp_policy_t *p) { if (p != NULL && p->deprecated_ekt != NULL) { @@ -2057,7 +2067,9 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx, } /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set stream (the pointer used in this function) */ stream = new_stream; @@ -2147,7 +2159,9 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set direction to outbound */ new_stream->direction = dir_srtp_sender; @@ -2745,7 +2759,9 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set stream (the pointer used in this function) */ stream = new_stream; @@ -2891,7 +2907,9 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) session->stream_template->direction = dir_srtp_receiver; break; case (ssrc_specific): - srtp_stream_list_insert(&session->stream_list, tmp); + status = srtp_insert_or_dealloc_stream(&session->stream_list, tmp, session->stream_template); + if (status) + return status; break; case (ssrc_undefined): default: @@ -3024,8 +3042,10 @@ static int update_template_stream(srtp_stream_t stream, void *raw_data) /* old / non-template streams are copied unchanged */ if (stream->session_keys[0].rtp_auth != session->stream_template->session_keys[0].rtp_auth) { - srtp_stream_list_delete(&session->stream_list, ssrc); - srtp_stream_list_insert(data->new_stream_list, stream); + stream = srtp_stream_list_delete(&session->stream_list, ssrc); + data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, session->stream_template); + if (data->status) + return 1; return 0; } @@ -3044,7 +3064,9 @@ static int update_template_stream(srtp_stream_t stream, void *raw_data) return 1; /* add new stream to the head of the new_stream_list */ - srtp_stream_list_insert(data->new_stream_list, stream); + data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, data->new_stream_template); + if (data->status) + return 1; /* restore old extended seq */ stream->rtp_rdbx.index = old_index; @@ -3877,7 +3899,9 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead( } /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set stream (the pointer used in this function) */ stream = new_stream; @@ -3942,7 +3966,9 @@ srtp_err_status_t srtp_protect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4388,7 +4414,9 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - srtp_stream_list_insert(&ctx->stream_list, new_stream); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + if (status) + return status; /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4752,13 +4780,15 @@ srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list) return srtp_err_status_ok; } -void srtp_stream_list_insert(srtp_stream_list_t *list_, srtp_stream_t stream) +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list_, srtp_stream_t stream) { srtp_stream_t *list = (srtp_stream_t *)list_; /* insert at the head of the list */ stream->next = (*list); (*list) = stream; + + return srtp_err_status_ok; } srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list_, uint32_t ssrc) From 0bc8b9bdaf52b92db9d409094c3375ea092a25d5 Mon Sep 17 00:00:00 2001 From: Alba Mendez Date: Mon, 12 Sep 2022 20:33:38 +0200 Subject: [PATCH 09/14] (format) --- srtp/srtp.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index 6e65f5348..c96080b6d 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -280,7 +280,9 @@ srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, } /* try to insert stream in list or deallocate it */ -static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t *list, srtp_stream_t stream, srtp_stream_t template) +static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t *list, + srtp_stream_t stream, + srtp_stream_t template) { srtp_err_status_t status = srtp_stream_list_insert(list, stream); /* on failure, ownership wasn't transferred and we need to deallocate */ @@ -2067,7 +2069,8 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx, } /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -2159,7 +2162,8 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream( + &ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -2759,7 +2763,8 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -2907,7 +2912,8 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) session->stream_template->direction = dir_srtp_receiver; break; case (ssrc_specific): - status = srtp_insert_or_dealloc_stream(&session->stream_list, tmp, session->stream_template); + status = srtp_insert_or_dealloc_stream(&session->stream_list, tmp, + session->stream_template); if (status) return status; break; @@ -3043,7 +3049,8 @@ static int update_template_stream(srtp_stream_t stream, void *raw_data) if (stream->session_keys[0].rtp_auth != session->stream_template->session_keys[0].rtp_auth) { stream = srtp_stream_list_delete(&session->stream_list, ssrc); - data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, session->stream_template); + data->status = srtp_insert_or_dealloc_stream( + data->new_stream_list, stream, session->stream_template); if (data->status) return 1; return 0; @@ -3064,7 +3071,8 @@ static int update_template_stream(srtp_stream_t stream, void *raw_data) return 1; /* add new stream to the head of the new_stream_list */ - data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, data->new_stream_template); + data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, + data->new_stream_template); if (data->status) return 1; @@ -3899,7 +3907,8 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead( } /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -3966,7 +3975,8 @@ srtp_err_status_t srtp_protect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream( + &ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -4414,7 +4424,8 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -4780,7 +4791,8 @@ srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list) return srtp_err_status_ok; } -srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list_, srtp_stream_t stream) +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list_, + srtp_stream_t stream) { srtp_stream_t *list = (srtp_stream_t *)list_; From 523c2a9cddc87b35053d6bce268308af6bc9174c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Thu, 15 Dec 2022 05:32:38 +0100 Subject: [PATCH 10/14] fix compile after merge --- srtp/srtp.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index 39234fc09..7ae448014 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -149,9 +149,8 @@ unsigned int srtp_get_version(void) return rv; } -static srtp_err_status_t srtp_stream_dealloc( - srtp_stream_ctx_t *stream, - const srtp_stream_ctx_t *stream_template) +srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, + const srtp_stream_ctx_t *stream_template) { srtp_err_status_t status; unsigned int i = 0; From 914cb552fbcc1854f5c88a969eaa34d22377e598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Fri, 23 Dec 2022 13:55:24 +0100 Subject: [PATCH 11/14] alternative stream list api and unit tests for the api Beside some small API rename the main difference is that the list implementation is not responsible for deallocating streams. Calling srtp_stream_list_dealloc() on a list that is not empty results in an error. This also changes the default implementation to be a double linked list making removing items faster. --- .github/workflows/cmake.yml | 8 ++ include/srtp_priv.h | 10 +- include/stream_list_priv.h | 34 ++--- srtp/srtp.c | 188 ++++++++++++++---------- test/srtp_driver.c | 280 ++++++++++++++++++++++++++++++++++-- 5 files changed, 404 insertions(+), 116 deletions(-) diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index ed914a637..5018fd2af 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -77,3 +77,11 @@ jobs: working-directory: ${{github.workspace}}/build shell: bash run: ctest + + - name: Test Stream List Replacement + working-directory: ${{github.workspace}}/build + shell: bash + run: | + make clean + make C_FLAGS="-DSRTP_NO_STREAM_LIST -DSRTP_USE_TEST_STREAM_LIST" srtp_driver + ./srtp_driver -v diff --git a/include/srtp_priv.h b/include/srtp_priv.h index 90045942d..f74afddb5 100644 --- a/include/srtp_priv.h +++ b/include/srtp_priv.h @@ -97,15 +97,6 @@ srtp_err_status_t srtp_steam_init_all_master_keys( srtp_master_key_t **keys, const unsigned int max_master_keys); -/* - * srtp_stream_init(s, p) initializes the srtp_stream_t s to - * use the policy at the location p - */ -srtp_err_status_t srtp_stream_init(srtp_stream_t srtp, const srtp_policy_t *p); - -srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, - const srtp_stream_ctx_t *stream_template); - /* * libsrtp internal datatypes */ @@ -154,6 +145,7 @@ typedef struct srtp_stream_ctx_t_ { int enc_xtn_hdr_count; uint32_t pending_roc; struct srtp_stream_ctx_t_ *next; /* linked list of streams */ + struct srtp_stream_ctx_t_ *prev; /* linked list of streams */ } strp_stream_ctx_t_; /* diff --git a/include/stream_list_priv.h b/include/stream_list_priv.h index 0245126a4..b44ca91e3 100644 --- a/include/stream_list_priv.h +++ b/include/stream_list_priv.h @@ -58,8 +58,8 @@ extern "C" { * the API was extracted to allow downstreams to override its * implementation by defining the `SRTP_NO_STREAM_LIST` preprocessor * directive, which removes the default implementation of these - * functions. if this is done, the `next` field is free for the - * implementation to use. + * functions. if this is done, the `next` & `prev` fields are free for + * the implementation to use. * * this is still an internal interface; there is no stability * guarantee--downstreams should watch this file for changes in @@ -69,7 +69,14 @@ extern "C" { /** * allocate and initialize a stream list instance */ -srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list); +srtp_err_status_t srtp_stream_list_alloc(srtp_stream_list_t *list_ptr); + +/** + * deallocate a stream list instance + * + * the list must be empty or else an error is returned. + */ +srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t list); /** * insert a stream into the list @@ -82,38 +89,33 @@ srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list); * behavior is undefined. if the SSRC field is mutated while the * stream is inserted, further operations have undefined behavior */ -srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list, +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t list, srtp_stream_t stream); /* * look up the stream corresponding to the specified SSRC and return it. * if no such SSRC is found, NULL is returned. */ -srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list, uint32_t ssrc); +srtp_stream_t srtp_stream_list_get(srtp_stream_list_t list, uint32_t ssrc); /** - * delete the stream associated to the specified SSRC. + * remove the stream from the list. * - * if a stream is found and removed, it's returned and ownership is - * transferred to the caller. if not found, NULL is returned. + * ownership is transferred to the caller. + * + * if the stream is not in the list the behavior is undefined. */ -srtp_stream_t srtp_stream_list_delete(srtp_stream_list_t *list, uint32_t ssrc); +void srtp_stream_list_remove(srtp_stream_list_t list, srtp_stream_t stream); /** * iterate through all stored streams. while iterating, it is allowed to delete * the current element; any other mutation to the list is undefined behavior. * returning non-zero from callback aborts the iteration. */ -void srtp_stream_list_for_each(srtp_stream_list_t *list, +void srtp_stream_list_for_each(srtp_stream_list_t list, int (*callback)(srtp_stream_t, void *), void *data); -/** - * deallocate a stream list instance and all streams inserted in it - */ -srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t *list, - srtp_stream_t template_); - #ifdef __cplusplus } #endif diff --git a/srtp/srtp.c b/srtp/srtp.c index 7ae448014..6ab5db476 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -149,8 +149,9 @@ unsigned int srtp_get_version(void) return rv; } -srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, - const srtp_stream_ctx_t *stream_template) +static srtp_err_status_t srtp_stream_dealloc( + srtp_stream_ctx_t *stream, + const srtp_stream_ctx_t *stream_template) { srtp_err_status_t status; unsigned int i = 0; @@ -280,7 +281,7 @@ srtp_err_status_t srtp_stream_dealloc(srtp_stream_ctx_t *stream, } /* try to insert stream in list or deallocate it */ -static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t *list, +static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t list, srtp_stream_t stream, srtp_stream_t template) { @@ -291,6 +292,34 @@ static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t *list, return status; } +struct remove_and_dealloc_streams_data { + srtp_err_status_t status; + srtp_stream_list_t list; + srtp_stream_t template; +}; + +static int remove_and_dealloc_streams_cb(srtp_stream_t stream, void *data) +{ + struct remove_and_dealloc_streams_data *d = + (struct remove_and_dealloc_streams_data *)data; + srtp_stream_list_remove(d->list, stream); + d->status = srtp_stream_dealloc(stream, d->template); + if (d->status) { + return 1; + } + return 0; +} + +static srtp_err_status_t srtp_remove_and_dealloc_streams( + srtp_stream_list_t list, + srtp_stream_t template) +{ + struct remove_and_dealloc_streams_data data = { srtp_err_status_ok, list, + template }; + srtp_stream_list_for_each(list, remove_and_dealloc_streams_cb, &data); + return data.status; +} + static srtp_err_status_t srtp_valid_policy(const srtp_policy_t *p) { if (p != NULL && p->deprecated_ekt != NULL) { @@ -565,6 +594,7 @@ static srtp_err_status_t srtp_stream_clone( /* defensive coding */ str->next = NULL; + str->prev = NULL; return srtp_err_status_ok; } @@ -1269,8 +1299,8 @@ srtp_err_status_t srtp_stream_init_keys(srtp_stream_ctx_t *srtp, return srtp_err_status_ok; } -srtp_err_status_t srtp_stream_init(srtp_stream_ctx_t *srtp, - const srtp_policy_t *p) +static srtp_err_status_t srtp_stream_init(srtp_stream_ctx_t *srtp, + const srtp_policy_t *p) { srtp_err_status_t err; @@ -2071,7 +2101,7 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx, } /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -2164,8 +2194,8 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream( - &ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -2765,7 +2795,7 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -2828,7 +2858,7 @@ srtp_err_status_t srtp_shutdown(void) srtp_stream_ctx_t *srtp_get_stream(srtp_t srtp, uint32_t ssrc) { - return srtp_stream_list_get(&srtp->stream_list, ssrc); + return srtp_stream_list_get(srtp->stream_list, ssrc); } srtp_err_status_t srtp_dealloc(srtp_t session) @@ -2842,8 +2872,8 @@ srtp_err_status_t srtp_dealloc(srtp_t session) */ /* deallocate streams */ - status = srtp_stream_list_dealloc(&session->stream_list, - session->stream_template); + status = srtp_remove_and_dealloc_streams(session->stream_list, + session->stream_template); if (status) return status; @@ -2854,6 +2884,12 @@ srtp_err_status_t srtp_dealloc(srtp_t session) return status; } + /* deallocate stream list */ + status = srtp_stream_list_dealloc(session->stream_list); + if (status) { + return status; + } + /* deallocate session context */ srtp_crypto_free(session); @@ -2914,7 +2950,7 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) session->stream_template->direction = dir_srtp_receiver; break; case (ssrc_specific): - status = srtp_insert_or_dealloc_stream(&session->stream_list, tmp, + status = srtp_insert_or_dealloc_stream(session->stream_list, tmp, session->stream_template); if (status) return status; @@ -2954,7 +2990,7 @@ srtp_err_status_t srtp_create(srtp_t *session, /* handle for session */ ctx->user_data = NULL; /* allocate stream list */ - stat = srtp_stream_list_create(&ctx->stream_list); + stat = srtp_stream_list_alloc(&ctx->stream_list); if (stat) { /* clean up everything */ srtp_dealloc(*session); @@ -2992,9 +3028,11 @@ srtp_err_status_t srtp_remove_stream(srtp_t session, uint32_t ssrc) return srtp_err_status_bad_param; /* find and remove stream from the list */ - stream = srtp_stream_list_delete(&session->stream_list, ssrc); - if (stream == NULL) + stream = srtp_stream_list_get(session->stream_list, ssrc); + if (stream == NULL) { return srtp_err_status_no_ctx; + } + srtp_stream_list_remove(session->stream_list, stream); /* deallocate the stream */ status = srtp_stream_dealloc(stream, session->stream_template); @@ -3035,10 +3073,10 @@ struct update_template_stream_data { srtp_err_status_t status; srtp_t session; srtp_stream_t new_stream_template; - srtp_stream_list_t *new_stream_list; + srtp_stream_list_t new_stream_list; }; -static int update_template_stream(srtp_stream_t stream, void *raw_data) +static int update_template_stream_cb(srtp_stream_t stream, void *raw_data) { struct update_template_stream_data *data = (struct update_template_stream_data *)raw_data; @@ -3050,7 +3088,7 @@ static int update_template_stream(srtp_stream_t stream, void *raw_data) /* old / non-template streams are copied unchanged */ if (stream->session_keys[0].rtp_auth != session->stream_template->session_keys[0].rtp_auth) { - stream = srtp_stream_list_delete(&session->stream_list, ssrc); + srtp_stream_list_remove(session->stream_list, stream); data->status = srtp_insert_or_dealloc_stream( data->new_stream_list, stream, session->stream_template); if (data->status) @@ -3115,7 +3153,7 @@ static srtp_err_status_t update_template_streams(srtp_t session, } /* allocate new stream list */ - status = srtp_stream_list_create(&new_stream_list); + status = srtp_stream_list_alloc(&new_stream_list); if (status) { srtp_crypto_free(new_stream_template); return status; @@ -3124,18 +3162,21 @@ static srtp_err_status_t update_template_streams(srtp_t session, /* process streams */ struct update_template_stream_data data = { srtp_err_status_ok, session, new_stream_template, - &new_stream_list }; - srtp_stream_list_for_each(&session->stream_list, &update_template_stream, + new_stream_list }; + srtp_stream_list_for_each(session->stream_list, update_template_stream_cb, &data); if (data.status) { /* free new allocations */ - srtp_stream_list_dealloc(&new_stream_list, new_stream_template); + srtp_remove_and_dealloc_streams(new_stream_list, new_stream_template); + srtp_stream_list_dealloc(new_stream_list); srtp_stream_dealloc(new_stream_template, NULL); return data.status; } /* dealloc old list / template */ - srtp_stream_list_dealloc(&session->stream_list, session->stream_template); + srtp_remove_and_dealloc_streams(session->stream_list, + session->stream_template); + srtp_stream_list_dealloc(session->stream_list); srtp_stream_dealloc(session->stream_template, NULL); /* set new list / template */ session->stream_template = new_stream_template; @@ -3909,7 +3950,7 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead( } /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -3977,8 +4018,8 @@ srtp_err_status_t srtp_protect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream( - &ctx->stream_list, new_stream, ctx->stream_template); + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, + ctx->stream_template); if (status) return status; @@ -4426,7 +4467,7 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx, return status; /* add new stream to the list */ - status = srtp_insert_or_dealloc_stream(&ctx->stream_list, new_stream, + status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); if (status) return status; @@ -4658,8 +4699,8 @@ srtp_err_status_t get_protect_trailer_length(srtp_t session, &data.length); } - srtp_stream_list_for_each(&session->stream_list, - &get_protect_trailer_length_cb, &data); + srtp_stream_list_for_each(session->stream_list, + get_protect_trailer_length_cb, &data); if (!data.valid) { return srtp_err_status_bad_param; @@ -4783,37 +4824,53 @@ srtp_err_status_t srtp_get_stream_roc(srtp_t session, #ifndef SRTP_NO_STREAM_LIST /* in the default implementation, we have an intrusive linked list */ -struct { - srtp_stream_ctx_t dummy; +struct srtp_stream_list_ctx_t_ { + srtp_stream_ctx_t data; } srtp_stream_list_ctx_t_; -srtp_err_status_t srtp_stream_list_create(srtp_stream_list_t *list) +srtp_err_status_t srtp_stream_list_alloc(srtp_stream_list_t *list_ptr) { - (*list) = NULL; + srtp_stream_list_t list = + srtp_crypto_alloc(sizeof(srtp_stream_list_ctx_t_)); + if (list == NULL) { + return srtp_err_status_alloc_fail; + } + *list_ptr = list; return srtp_err_status_ok; } -srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t *list_, - srtp_stream_t stream) +srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t list) { - srtp_stream_t *list = (srtp_stream_t *)list_; + /* list must be empty */ + if (list->data.next) { + return srtp_err_status_fail; + } + srtp_crypto_free(list); + return srtp_err_status_ok; +} +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t list, + srtp_stream_t stream) +{ /* insert at the head of the list */ - stream->next = (*list); - (*list) = stream; + stream->next = list->data.next; + if (stream->next != NULL) { + stream->next->prev = stream; + } + list->data.next = stream; + stream->prev = &(list->data); return srtp_err_status_ok; } -srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list_, uint32_t ssrc) +srtp_stream_t srtp_stream_list_get(srtp_stream_list_t list, uint32_t ssrc) { - srtp_stream_t *list = (srtp_stream_t *)list_; - /* walk down list until ssrc is found */ - srtp_stream_t stream = (*list); + srtp_stream_t stream = list->data.next; while (stream != NULL) { - if (stream->ssrc == ssrc) + if (stream->ssrc == ssrc) { return stream; + } stream = stream->next; } @@ -4821,31 +4878,22 @@ srtp_stream_t srtp_stream_list_get(srtp_stream_list_t *list_, uint32_t ssrc) return NULL; } -srtp_stream_t srtp_stream_list_delete(srtp_stream_list_t *list_, uint32_t ssrc) +void srtp_stream_list_remove(srtp_stream_list_t list, + srtp_stream_t stream_to_remove) { - srtp_stream_t *list = (srtp_stream_t *)list_; + (void)list; - /* walk down list until ssrc is found */ - srtp_stream_t *stream = list; - while ((*stream) != NULL) { - if ((*stream)->ssrc == ssrc) { - srtp_stream_t tmp = (*stream); - (*stream) = tmp->next; - return tmp; - } - stream = &(*stream)->next; + stream_to_remove->prev->next = stream_to_remove->next; + if (stream_to_remove->next != NULL) { + stream_to_remove->next->prev = stream_to_remove->prev; } - - /* we haven't found our ssrc, so return a null */ - return NULL; } -void srtp_stream_list_for_each(srtp_stream_list_t *list_, +void srtp_stream_list_for_each(srtp_stream_list_t list, int (*callback)(srtp_stream_t, void *), void *data) { - srtp_stream_t *list = (srtp_stream_t *)list_; - srtp_stream_t stream = (*list); + srtp_stream_t stream = list->data.next; while (stream != NULL) { srtp_stream_t tmp = stream; stream = stream->next; @@ -4854,22 +4902,4 @@ void srtp_stream_list_for_each(srtp_stream_list_t *list_, } } -srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t *list_, - srtp_stream_t template) -{ - srtp_stream_t *list = (srtp_stream_t *)list_; - srtp_err_status_t status; - - /* walk list of streams, deallocating as we go */ - while ((*list) != NULL) { - srtp_stream_t next = (*list)->next; - status = srtp_stream_dealloc((*list), template); - if (status) - return status; - (*list) = next; - } - - return srtp_err_status_ok; -} - #endif diff --git a/test/srtp_driver.c b/test/srtp_driver.c index f6e395755..8ecf273a1 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -125,6 +125,8 @@ char *srtp_packet_to_string(srtp_hdr_t *hdr, int packet_len); double mips_estimate(int num_trials, int *ignore); +srtp_err_status_t srtp_stream_list_test(void); + #define TEST_MKI_ID_SIZE 4 extern uint8_t test_key[46]; @@ -154,15 +156,17 @@ srtp_master_key_t *test_keys[2] = { void usage(char *prog_name) { - printf("usage: %s [ -t ][ -c ][ -v ][ -o ][-d ]* [ -l ]\n" - " -t run timing test\n" - " -r run rejection timing test\n" - " -c run codec timing test\n" - " -v run validation tests\n" - " -o output logging to stdout\n" - " -d turn on debugging module \n" - " -l list debugging modules\n", - prog_name); + printf( + "usage: %s [ -t ][ -c ][ -v ][ -s ][ -o ][-d ]* [ -l ]\n" + " -t run timing test\n" + " -r run rejection timing test\n" + " -c run codec timing test\n" + " -v run validation tests\n" + " -s run stream list tests only\n" + " -o output logging to stdout\n" + " -d turn on debugging module \n" + " -l list debugging modules\n", + prog_name); exit(1); } @@ -218,6 +222,7 @@ int main(int argc, char *argv[]) unsigned do_rejection_test = 0; unsigned do_codec_timing = 0; unsigned do_validation = 0; + unsigned do_stream_list = 0; unsigned do_list_mods = 0; unsigned do_log_stdout = 0; srtp_err_status_t status; @@ -252,7 +257,7 @@ int main(int argc, char *argv[]) /* process input arguments */ while (1) { - q = getopt_s(argc, argv, "trcvold:"); + q = getopt_s(argc, argv, "trcvsold:"); if (q == -1) { break; } @@ -268,6 +273,10 @@ int main(int argc, char *argv[]) break; case 'v': do_validation = 1; + do_stream_list = 1; + break; + case 's': + do_stream_list = 1; break; case 'o': do_log_stdout = 1; @@ -288,7 +297,7 @@ int main(int argc, char *argv[]) } if (!do_validation && !do_timing_test && !do_codec_timing && - !do_list_mods && !do_rejection_test) { + !do_list_mods && !do_rejection_test && !do_stream_list) { usage(argv[0]); } @@ -598,6 +607,16 @@ int main(int argc, char *argv[]) } } + if (do_stream_list) { + printf("testing srtp_stream_list..."); + if (srtp_stream_list_test() == srtp_err_status_ok) { + printf("passed\n"); + } else { + printf("failed\n"); + exit(1); + } + } + if (do_timing_test) { const srtp_policy_t **policy = policy_array; @@ -1551,7 +1570,7 @@ srtp_err_status_t srtp_session_print_policy(srtp_t srtp) /* loop over streams in session, printing the policy of each */ data.is_template = 0; - srtp_stream_list_for_each(&srtp->stream_list, &srtp_session_print_stream, + srtp_stream_list_for_each(srtp->stream_list, srtp_session_print_stream, &data); return data.status; @@ -4280,3 +4299,240 @@ const srtp_policy_t wildcard_policy = { 0, /* list of encrypted extension headers is empty */ NULL }; + +static srtp_stream_t stream_list_test_create_stream(uint32_t ssrc) +{ + srtp_stream_t stream = calloc(1, sizeof(srtp_stream_ctx_t)); + stream->ssrc = ssrc; + return stream; +} + +static void stream_list_test_free_stream(srtp_stream_t stream) +{ + free(stream); +} + +int stream_list_test_count_cb(srtp_stream_t stream, void *data) +{ + int *count = (int *)data; + (*count)++; + (void)stream; + return 0; +} + +struct remove_one_data { + uint32_t ssrc; + srtp_stream_list_t list; +}; + +int stream_list_test_remove_one_cb(srtp_stream_t stream, void *data) +{ + struct remove_one_data *d = (struct remove_one_data *)data; + if (stream->ssrc == d->ssrc) { + srtp_stream_list_remove(d->list, stream); + stream_list_test_free_stream(stream); + return 1; + } + return 0; +} + +int stream_list_test_remove_all_cb(srtp_stream_t stream, void *data) +{ + srtp_stream_list_t *list = (srtp_stream_list_t *)data; + srtp_stream_list_remove(*list, stream); + stream_list_test_free_stream(stream); + return 0; +} + +srtp_err_status_t srtp_stream_list_test(void) +{ + srtp_stream_list_t list; + + if (srtp_stream_list_alloc(&list)) { + return srtp_err_status_fail; + } + + /* add 4 streams*/ + if (srtp_stream_list_insert(list, stream_list_test_create_stream(1))) { + return srtp_err_status_fail; + } + if (srtp_stream_list_insert(list, stream_list_test_create_stream(2))) { + return srtp_err_status_fail; + } + if (srtp_stream_list_insert(list, stream_list_test_create_stream(3))) { + return srtp_err_status_fail; + } + if (srtp_stream_list_insert(list, stream_list_test_create_stream(4))) { + return srtp_err_status_fail; + } + + /* find */ + if (srtp_stream_list_get(list, 3) == NULL) { + return srtp_err_status_fail; + } + if (srtp_stream_list_get(list, 1) == NULL) { + return srtp_err_status_fail; + } + if (srtp_stream_list_get(list, 2) == NULL) { + return srtp_err_status_fail; + } + if (srtp_stream_list_get(list, 4) == NULL) { + return srtp_err_status_fail; + } + + /* find not in list */ + if (srtp_stream_list_get(list, 5)) { + return srtp_err_status_fail; + } + + /* for each */ + int count = 0; + srtp_stream_list_for_each(list, stream_list_test_count_cb, &count); + if (count != 4) { + return srtp_err_status_fail; + } + + /* remove */ + srtp_stream_t stream = srtp_stream_list_get(list, 3); + if (stream == NULL) { + return srtp_err_status_fail; + } + srtp_stream_list_remove(list, stream); + stream_list_test_free_stream(stream); + + /* find after remove */ + if (srtp_stream_list_get(list, 3)) { + return srtp_err_status_fail; + } + + /* recount */ + count = 0; + srtp_stream_list_for_each(list, stream_list_test_count_cb, &count); + if (count != 3) { + return srtp_err_status_fail; + } + + /* remove one in for each */ + struct remove_one_data data = { 2, list }; + srtp_stream_list_for_each(list, stream_list_test_remove_one_cb, &data); + + /* find after remove */ + if (srtp_stream_list_get(list, 2)) { + return srtp_err_status_fail; + } + + /* recount */ + count = 0; + srtp_stream_list_for_each(list, stream_list_test_count_cb, &count); + if (count != 2) { + return srtp_err_status_fail; + } + + /* destroy non empty list */ + if (srtp_stream_list_dealloc(list) == srtp_err_status_ok) { + return srtp_err_status_fail; + } + + /* remove all in for each */ + srtp_stream_list_for_each(list, stream_list_test_remove_all_cb, &list); + + /* recount */ + count = 0; + srtp_stream_list_for_each(list, stream_list_test_count_cb, &count); + if (count != 0) { + return srtp_err_status_fail; + } + + /* destroy empty list */ + if (srtp_stream_list_dealloc(list)) { + return srtp_err_status_fail; + } + + return srtp_err_status_ok; +} + +#ifdef SRTP_USE_TEST_STREAM_LIST + +/* + * A srtp_stream_list_ctx_t implementation using a single linked list + * that does not use the internal next / prev fields. + */ + +struct test_list_node { + srtp_stream_t stream; + struct test_list_node *next; +}; +struct srtp_stream_list_ctx_t_ { + struct test_list_node *head; +}; + +srtp_err_status_t srtp_stream_list_alloc(srtp_stream_list_t *list_ptr) +{ + struct srtp_stream_list_ctx_t_ *l = + malloc(sizeof(struct srtp_stream_list_ctx_t_)); + l->head = NULL; + *list_ptr = l; + return srtp_err_status_ok; +} + +srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t list) +{ + struct test_list_node *node = list->head; + if (node) { + return srtp_err_status_fail; + } + free(list); + + return srtp_err_status_ok; +} + +srtp_err_status_t srtp_stream_list_insert(srtp_stream_list_t list, + srtp_stream_t stream) +{ + struct test_list_node *node = malloc(sizeof(struct test_list_node)); + node->stream = stream; + node->next = list->head; + list->head = node; + + return srtp_err_status_ok; +} + +srtp_stream_t srtp_stream_list_get(srtp_stream_list_t list, uint32_t ssrc) +{ + struct test_list_node *node = list->head; + while (node != NULL) { + if (node->stream->ssrc == ssrc) + return node->stream; + node = node->next; + } + return NULL; +} + +void srtp_stream_list_remove(srtp_stream_list_t list, srtp_stream_t stream) +{ + struct test_list_node **node = &(list->head); + while ((*node) != NULL) { + if ((*node)->stream->ssrc == stream->ssrc) { + struct test_list_node *tmp = (*node); + (*node) = tmp->next; + free(tmp); + return; + } + node = &(*node)->next; + } +} + +void srtp_stream_list_for_each(srtp_stream_list_t list, + int (*callback)(srtp_stream_t, void *), + void *data) +{ + struct test_list_node *node = list->head; + while (node != NULL) { + struct test_list_node *tmp = node; + node = node->next; + if (callback(tmp->stream, data)) + break; + } +} + +#endif From 8ef408485cee69e28a9e758f01696290dd94d059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Tue, 10 Jan 2023 12:17:14 +0100 Subject: [PATCH 12/14] use cmake to build and run test This does it in a cross platform manor. --- .github/workflows/cmake.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 5018fd2af..00a96a2a6 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -82,6 +82,6 @@ jobs: working-directory: ${{github.workspace}}/build shell: bash run: | - make clean - make C_FLAGS="-DSRTP_NO_STREAM_LIST -DSRTP_USE_TEST_STREAM_LIST" srtp_driver - ./srtp_driver -v + cmake $GITHUB_WORKSPACE -DBUILD_WITH_SANITIZERS=TRUE ${{ matrix.cmake-crypto-enable}} ${{env.cmake-crypto-dir}} -DCMAKE_C_FLAGS:STRING="-DSRTP_NO_STREAM_LIST -DSRTP_USE_TEST_STREAM_LIST" + cmake --build . --clean-first -t srtp_driver + ctest -R srtp_driver From 07334b277cb2cc9ca30f087f022f57e9f208e3a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Thu, 12 Jan 2023 18:20:11 +0100 Subject: [PATCH 13/14] move stream list replace test to own CI work flow --- .github/workflows/cmake.yml | 8 ------ .github/workflows/stream_list.yml | 41 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 .github/workflows/stream_list.yml diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 00a96a2a6..ed914a637 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -77,11 +77,3 @@ jobs: working-directory: ${{github.workspace}}/build shell: bash run: ctest - - - name: Test Stream List Replacement - working-directory: ${{github.workspace}}/build - shell: bash - run: | - cmake $GITHUB_WORKSPACE -DBUILD_WITH_SANITIZERS=TRUE ${{ matrix.cmake-crypto-enable}} ${{env.cmake-crypto-dir}} -DCMAKE_C_FLAGS:STRING="-DSRTP_NO_STREAM_LIST -DSRTP_USE_TEST_STREAM_LIST" - cmake --build . --clean-first -t srtp_driver - ctest -R srtp_driver diff --git a/.github/workflows/stream_list.yml b/.github/workflows/stream_list.yml new file mode 100644 index 000000000..7da047b6b --- /dev/null +++ b/.github/workflows/stream_list.yml @@ -0,0 +1,41 @@ +name: Stream List CI + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + +jobs: + build: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + + runs-on: ${{ matrix.os }} + + env: + CTEST_OUTPUT_ON_FAILURE: 1 + + steps: + + - uses: actions/checkout@v2 + + - name: Create Build Environment + run: cmake -E make_directory ${{github.workspace}}/build + + - name: Configure CMake + working-directory: ${{github.workspace}}/build + shell: bash + run: cmake $GITHUB_WORKSPACE -DBUILD_WITH_SANITIZERS=TRUE -DCMAKE_C_FLAGS:STRING="-DSRTP_NO_STREAM_LIST -DSRTP_USE_TEST_STREAM_LIST" + + - name: Build + working-directory: ${{github.workspace}}/build + shell: bash + run: cmake --build . -t srtp_driver + + - name: Test + working-directory: ${{github.workspace}}/build + shell: bash + run: ctest -R srtp_driver From d1eb03abfe712f223e9cb3ded1c1392e827aa981 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Thu, 12 Jan 2023 19:16:00 +0100 Subject: [PATCH 14/14] address comments from review, no function change --- include/srtp_priv.h | 10 ++++-- include/stream_list_priv.h | 7 ++-- srtp/srtp.c | 65 ++++++++++++++++++++++++++------------ test/srtp_driver.c | 2 +- 4 files changed, 56 insertions(+), 28 deletions(-) diff --git a/include/srtp_priv.h b/include/srtp_priv.h index f74afddb5..8e7848989 100644 --- a/include/srtp_priv.h +++ b/include/srtp_priv.h @@ -144,8 +144,14 @@ typedef struct srtp_stream_ctx_t_ { int *enc_xtn_hdr; int enc_xtn_hdr_count; uint32_t pending_roc; - struct srtp_stream_ctx_t_ *next; /* linked list of streams */ - struct srtp_stream_ctx_t_ *prev; /* linked list of streams */ + /* + The next and prev pointers are here to allow for a stream list to be + implemented as an intrusive doubly-linked list (the former being the + default). Other stream list implementations can ignore these fields or use + them for some other purpose specific to the stream list implementation. + */ + struct srtp_stream_ctx_t_ *next; + struct srtp_stream_ctx_t_ *prev; } strp_stream_ctx_t_; /* diff --git a/include/stream_list_priv.h b/include/stream_list_priv.h index b44ca91e3..f49cd551d 100644 --- a/include/stream_list_priv.h +++ b/include/stream_list_priv.h @@ -83,7 +83,6 @@ srtp_err_status_t srtp_stream_list_dealloc(srtp_stream_list_t list); * * returns srtp_err_status_alloc_fail if insertion failed due to unavailable * capacity in the list. if operation succeeds, srtp_err_status_ok is returned - * and ownership is transferred from caller into the list * * if another stream with the same SSRC already exists in the list, * behavior is undefined. if the SSRC field is mutated while the @@ -101,9 +100,9 @@ srtp_stream_t srtp_stream_list_get(srtp_stream_list_t list, uint32_t ssrc); /** * remove the stream from the list. * - * ownership is transferred to the caller. - * - * if the stream is not in the list the behavior is undefined. + * The stream to be removed is referenced "by value", i.e., by the pointer to be + * removed from the list. This pointer is obtained using `srtp_stream_list_get` + * or as callback parameter in `srtp_stream_list_for_each`. */ void srtp_stream_list_remove(srtp_stream_list_t list, srtp_stream_t stream); diff --git a/srtp/srtp.c b/srtp/srtp.c index 6ab5db476..933f58f96 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -287,8 +287,9 @@ static srtp_err_status_t srtp_insert_or_dealloc_stream(srtp_stream_list_t list, { srtp_err_status_t status = srtp_stream_list_insert(list, stream); /* on failure, ownership wasn't transferred and we need to deallocate */ - if (status) + if (status) { srtp_stream_dealloc(stream, template); + } return status; } @@ -2103,8 +2104,9 @@ static srtp_err_status_t srtp_unprotect_aead(srtp_ctx_t *ctx, /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set stream (the pointer used in this function) */ stream = new_stream; @@ -2196,8 +2198,9 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx, /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set direction to outbound */ new_stream->direction = dir_srtp_sender; @@ -2791,14 +2794,16 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx, */ status = srtp_stream_clone(ctx->stream_template, hdr->ssrc, &new_stream); - if (status) + if (status) { return status; + } /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set stream (the pointer used in this function) */ stream = new_stream; @@ -2874,14 +2879,16 @@ srtp_err_status_t srtp_dealloc(srtp_t session) /* deallocate streams */ status = srtp_remove_and_dealloc_streams(session->stream_list, session->stream_template); - if (status) + if (status) { return status; + } /* deallocate stream template, if there is one */ if (session->stream_template != NULL) { status = srtp_stream_dealloc(session->stream_template, NULL); - if (status) + if (status) { return status; + } } /* deallocate stream list */ @@ -2952,8 +2959,9 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) case (ssrc_specific): status = srtp_insert_or_dealloc_stream(session->stream_list, tmp, session->stream_template); - if (status) + if (status) { return status; + } break; case (ssrc_undefined): default: @@ -3032,6 +3040,7 @@ srtp_err_status_t srtp_remove_stream(srtp_t session, uint32_t ssrc) if (stream == NULL) { return srtp_err_status_no_ctx; } + srtp_stream_list_remove(session->stream_list, stream); /* deallocate the stream */ @@ -3091,8 +3100,9 @@ static int update_template_stream_cb(srtp_stream_t stream, void *raw_data) srtp_stream_list_remove(session->stream_list, stream); data->status = srtp_insert_or_dealloc_stream( data->new_stream_list, stream, session->stream_template); - if (data->status) + if (data->status) { return 1; + } return 0; } @@ -3102,19 +3112,22 @@ static int update_template_stream_cb(srtp_stream_t stream, void *raw_data) /* remove stream */ data->status = srtp_remove_stream(session, ssrc); - if (data->status) + if (data->status) { return 1; + } /* allocate and initialize a new stream */ data->status = srtp_stream_clone(data->new_stream_template, ssrc, &stream); - if (data->status) + if (data->status) { return 1; + } /* add new stream to the head of the new_stream_list */ data->status = srtp_insert_or_dealloc_stream(data->new_stream_list, stream, data->new_stream_template); - if (data->status) + if (data->status) { return 1; + } /* restore old extended seq */ stream->rtp_rdbx.index = old_index; @@ -3178,6 +3191,7 @@ static srtp_err_status_t update_template_streams(srtp_t session, session->stream_template); srtp_stream_list_dealloc(session->stream_list); srtp_stream_dealloc(session->stream_template, NULL); + /* set new list / template */ session->stream_template = new_stream_template; session->stream_list = new_stream_list; @@ -3952,8 +3966,9 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead( /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4020,8 +4035,9 @@ srtp_err_status_t srtp_protect_rtcp_mki(srtp_t ctx, /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4469,8 +4485,9 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx, /* add new stream to the list */ status = srtp_insert_or_dealloc_stream(ctx->stream_list, new_stream, ctx->stream_template); - if (status) + if (status) { return status; + } /* set stream (the pointer used in this function) */ stream = new_stream; @@ -4652,8 +4669,8 @@ srtp_err_status_t stream_get_protect_trailer_length(srtp_stream_ctx_t *stream, } struct get_protect_trailer_length_data { - int valid; - uint32_t length; + uint32_t found_stream; /* whether at least one matching stream was found */ + uint32_t length; /* maximum trailer length found so far */ uint32_t is_rtp; uint32_t use_mki; uint32_t mki_index; @@ -4665,10 +4682,10 @@ static int get_protect_trailer_length_cb(srtp_stream_t stream, void *raw_data) (struct get_protect_trailer_length_data *)raw_data; uint32_t temp_length; - data->valid = 1; if (stream_get_protect_trailer_length(stream, data->is_rtp, data->use_mki, data->mki_index, &temp_length) == srtp_err_status_ok) { + data->found_stream = 1; if (temp_length > data->length) { data->length = temp_length; } @@ -4694,7 +4711,7 @@ srtp_err_status_t get_protect_trailer_length(srtp_t session, stream = session->stream_template; if (stream != NULL) { - data.valid = 1; + data.found_stream = 1; stream_get_protect_trailer_length(stream, is_rtp, use_mki, mki_index, &data.length); } @@ -4702,7 +4719,7 @@ srtp_err_status_t get_protect_trailer_length(srtp_t session, srtp_stream_list_for_each(session->stream_list, get_protect_trailer_length_cb, &data); - if (!data.valid) { + if (!data.found_stream) { return srtp_err_status_bad_param; } @@ -4823,8 +4840,10 @@ srtp_err_status_t srtp_get_stream_roc(srtp_t session, #ifndef SRTP_NO_STREAM_LIST -/* in the default implementation, we have an intrusive linked list */ +/* in the default implementation, we have an intrusive doubly-linked list */ struct srtp_stream_list_ctx_t_ { + /* a stub stream that just holds pointers to the beginning and end of the + * list */ srtp_stream_ctx_t data; } srtp_stream_list_ctx_t_; @@ -4835,6 +4854,10 @@ srtp_err_status_t srtp_stream_list_alloc(srtp_stream_list_t *list_ptr) if (list == NULL) { return srtp_err_status_alloc_fail; } + + list->data.next = NULL; + list->data.prev = NULL; + *list_ptr = list; return srtp_err_status_ok; } diff --git a/test/srtp_driver.c b/test/srtp_driver.c index 8ecf273a1..42229be78 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -4302,7 +4302,7 @@ const srtp_policy_t wildcard_policy = { static srtp_stream_t stream_list_test_create_stream(uint32_t ssrc) { - srtp_stream_t stream = calloc(1, sizeof(srtp_stream_ctx_t)); + srtp_stream_t stream = malloc(sizeof(srtp_stream_ctx_t)); stream->ssrc = ssrc; return stream; }