Add runtime string checks and additional unit tests

Author: sirknightj

.github/workflows/kvssink.yml

@@ -65,4 +65,4 @@ jobs:
         working-directory: ./build
         run: |
           export GST_PLUGIN_PATH=`pwd`
-          ./tst/gstkvsplugintest 
+          GST_DEBUG=4 ./tst/gstkvsplugintest 

src/gstreamer/gstkvssink.cpp

@@ -137,6 +137,8 @@ GST_DEBUG_CATEGORY_STATIC (gst_kvs_sink_debug);
 
 #define MAX_GSTREAMER_MEDIA_TYPE_LEN    16
 
+#define INTERNAL_CHECK_PREFIX           "Assertion failed:"
+
 namespace KvsSinkSignals {
     guint err_signal_id;
     guint ack_signal_id;
@@ -261,6 +263,11 @@ void closed(UINT64 custom_data, STREAM_HANDLE stream_handle, UPLOAD_HANDLE uploa
 void kinesis_video_producer_init(GstKvsSink *kvssink)
 {
     auto data = kvssink->data;
+
+    if (kvssink->stream_name == NULL) {
+        throw runtime_error(string(INTERNAL_CHECK_PREFIX) + " kvssink->stream_name is unexpectedly NULL!");
+    }
+
     unique_ptr<DeviceInfoProvider> device_info_provider(new KvsSinkDeviceInfoProvider(kvssink->storage_size,
                                                         kvssink->stop_stream_timeout,
                                                         kvssink->service_connection_timeout,
@@ -302,11 +309,11 @@ void kinesis_video_producer_init(GstKvsSink *kvssink)
         }
 
     } else {
-        access_key_str = string(kvssink->access_key);
-        secret_key_str = string(kvssink->secret_key);
+        access_key_str = kvssink->access_key ? string(kvssink->access_key) : "";
+        secret_key_str = kvssink->secret_key ? string(kvssink->secret_key) : "";
     }
 
-    // Handle session token seperately, since this is optional with long term credentials
+    // Handle session token separately, since this is optional with long term credentials
     if (0 == strcmp(kvssink->session_token, DEFAULT_SESSION_TOKEN)) {
         session_token_str = "";
         if (nullptr != (session_token = getenv(SESSION_TOKEN_ENV_VAR))) {
@@ -370,6 +377,11 @@ void kinesis_video_producer_init(GstKvsSink *kvssink)
         LOG_INFO("Getting URL from env for " << kvssink->stream_name);
         control_plane_uri_str = string(control_plane_uri);
     }
+
+    if (kvssink->user_agent == NULL) {
+        throw runtime_error(string(INTERNAL_CHECK_PREFIX) + " kvssink->user_agent is unexpectedly NULL!");
+    }
+
     LOG_INFO("User agent string: " << kvssink->user_agent);
     data->kinesis_video_producer = KinesisVideoProducer::createSync(std::move(device_info_provider),
                                                                     std::move(client_callback_provider),
@@ -423,6 +435,13 @@ void create_kinesis_video_stream(GstKvsSink *kvssink) {
             break;
     }
 
+    // StreamDefinition takes in C++ strings, check the gchar* for nullptr before constructing
+    if (kvssink->stream_name == NULL || kvssink->content_type == NULL ||
+        kvssink->user_agent == NULL || kvssink->kms_key_id == NULL ||
+        kvssink->codec_id == NULL || kvssink->track_name == NULL) {
+        throw runtime_error(string(INTERNAL_CHECK_PREFIX) + " A string was unexpectedly NULL!");
+    }
+
     unique_ptr<StreamDefinition> stream_definition(new StreamDefinition(kvssink->stream_name,
             hours(kvssink->retention_period_hours),
             p_stream_tags,
@@ -476,6 +495,15 @@ bool kinesis_video_stream_init(GstKvsSink *kvssink, string &err_msg) {
             create_kinesis_video_stream(kvssink);
             break;
         } catch (runtime_error &err) {
+
+            // Don't retry if the error is an internal error
+            if (STRNCMP(INTERNAL_CHECK_PREFIX, err.what(), STRLEN(INTERNAL_CHECK_PREFIX)) == 0) {
+                ostringstream oss;
+                oss << "Failed to create stream. Error: " << err.what();
+                err_msg = oss.str();
+                return false;
+            }
+
             if (--retry_count == 0) {
                 ostringstream oss;
                 oss << "Failed to create stream. Error: " << err.what();
@@ -1569,6 +1597,10 @@ init_track_data(GstKvsSink *kvssink) {
 
     g_free(video_content_type);
     g_free(audio_content_type);
+
+    if (kvssink->content_type == NULL) {
+        throw runtime_error(string(INTERNAL_CHECK_PREFIX) + " kvssink->content_type is unexpectedly NULL!");
+    }
 }
 
 static GstStateChangeReturn

tst/gstreamer/gstkvstest.cpp

@@ -17,8 +17,9 @@ static char const *sessionToken;
 static GstElement *
 setup_kinesisvideoproducersink(void)
 {
+    cout << "setup_kinesisvideoproducersink() start" << endl;
+
     GstElement *kinesisvideoproducersink;
-    GST_DEBUG("Setup kinesisvideoproducersink");
     kinesisvideoproducersink = gst_check_setup_element ("kvssink");
     fail_unless(kinesisvideoproducersink != nullptr, "Failed to create kvssink element (is GST_PLUGIN_PATH set?)");
 
@@ -30,23 +31,28 @@ setup_kinesisvideoproducersink(void)
 
     // https://gitlab.freedesktop.org/gstreamer/gst-docs/-/issues/91
     // Use gst_element_request_pad_simple in newer GStreamer versions
-    GstPad *sinkpad = gst_element_get_request_pad(kinesisvideoproducersink, "video_0");
+    GstPad *sinkpad = gst_element_get_request_pad(kinesisvideoproducersink, "video_%u");
     fail_unless(sinkpad != nullptr, "Failed to request video pad");
     gst_object_unref(sinkpad);
 
+    cout << "setup_kinesisvideoproducersink() end" << endl;
     return kinesisvideoproducersink;
 }
 
 static void
 cleanup_kinesisvideoproducersink(GstElement * kinesisvideoproducersink)
 {
+    cout << "cleanup_kinesisvideoproducersink() start" << endl;
+
     GstPad *sinkpad = gst_element_get_static_pad(kinesisvideoproducersink, "video_0");
     if (sinkpad) {
         gst_element_release_request_pad(kinesisvideoproducersink, sinkpad);
         gst_object_unref(sinkpad);
     }
 
     gst_check_teardown_element (kinesisvideoproducersink);
+
+    cout << "cleanup_kinesisvideoproducersink() end" << endl;
 }
 
 GST_START_TEST(kvsproducersinktestplayandstop)
@@ -101,6 +107,49 @@ GST_START_TEST(kvsproducersinkteststop)
     }
 GST_END_TEST;
 
+GST_START_TEST(check_stream_name_null_handled)
+    {
+        GstElement *kinesisvideoproducersink;
+
+        // Setup
+        kinesisvideoproducersink = gst_check_setup_element ("kvssink");
+        fail_unless(kinesisvideoproducersink != nullptr, "Failed to create kvssink element");
+
+        g_object_set(G_OBJECT (kinesisvideoproducersink),
+            "stream-name", nullptr,
+            NULL);
+
+        // Test - Initialization of the client & stream occurs here
+        fail_unless_equals_int(GST_STATE_CHANGE_FAILURE, gst_element_set_state(kinesisvideoproducersink, GST_STATE_PLAYING));
+
+        // Teardown
+        fail_unless_equals_int(GST_STATE_CHANGE_SUCCESS, gst_element_set_state(kinesisvideoproducersink, GST_STATE_NULL));
+        gst_check_teardown_element(kinesisvideoproducersink);
+    }
+GST_END_TEST;
+
+GST_START_TEST(check_no_pads_content_type_set_correctly)
+    {
+        GstElement *kinesisvideoproducersink;
+
+        // Setup
+        kinesisvideoproducersink = gst_check_setup_element ("kvssink");
+        fail_unless(kinesisvideoproducersink != nullptr, "Failed to create kvssink element");
+
+        g_object_set(G_OBJECT (kinesisvideoproducersink),
+            "stream-name", "test-stream",
+            NULL);
+
+        // Test - Initialization of the client & stream occurs here
+        // Expecting kvssink->content_type == nullptr since no pads attached
+        fail_unless_equals_int(GST_STATE_CHANGE_FAILURE, gst_element_set_state(kinesisvideoproducersink, GST_STATE_PLAYING));
+
+        // Teardown
+        fail_unless_equals_int(GST_STATE_CHANGE_SUCCESS, gst_element_set_state(kinesisvideoproducersink, GST_STATE_NULL));
+        gst_check_teardown_element(kinesisvideoproducersink);
+    }
+GST_END_TEST;
+
 GST_START_TEST(check_properties_are_passed_correctly)
     {
         GstElement *pElement =
@@ -266,6 +315,8 @@ Suite *gst_kinesisvideoproducer_suite(void) {
         return s;
     }
 
+    tcase_add_test(tc, check_stream_name_null_handled);
+    tcase_add_test(tc, check_no_pads_content_type_set_correctly);
     tcase_add_test(tc, kvsproducersinktestplayandstop);
     tcase_add_test(tc, kvsproducersinktestplaytopausetoplay);
     tcase_add_test(tc, kvsproducersinkteststop);