Fix AV in logging when classification array isn't valid and add tests. (#1174)

* Fix AV in logging when classification array isn't valid and add tests.

* Revert cmakesettings json change.

* Remove unnecessary assert, to docouple sequence of test runs.

* Update null to empty.

* Update tests for when logging is turned off.

* Go back to for-loop instead of while-loop.

* Add another success test for logging.

* Move the validation to a precondition check.

* Rename test.

* Address PR feedback and put ifdef around validation method.

* Apply rename across the tests.

sdk/inc/azure/core/az_log.h

@@ -33,6 +33,8 @@
 /**
  * @brief Identifies the classifications of log messages produced by the SDK.
  */
+// Make sure to update #_az_log_classifications_are_valid() whenever a new value is added to this
+// enum.
 typedef enum
 {
   AZ_LOG_END_OF_LIST

sdk/src/azure/core/az_log.c

@@ -19,8 +19,40 @@
 static az_log_classification const* volatile _az_log_classifications = NULL;
 static az_log_message_fn volatile _az_log_message_callback = NULL;
 
+// Verifies that the classification that the user provided is one of the valid possibilties and
+// guards against looping past the end of the classification array.
+// Make sure to update the switch statement whenever new classifications are added.
+#ifndef AZ_NO_PRECONDITION_CHECKING
+AZ_INLINE bool _az_log_classifications_are_valid(az_log_classification const* classifications)
+{
+  if (classifications == NULL)
+  {
+    return true;
+  }
+  for (az_log_classification const* cls = classifications; *cls != AZ_LOG_END_OF_LIST; ++cls)
+  {
+    switch (*cls)
+    {
+      case AZ_LOG_HTTP_REQUEST:
+      case AZ_LOG_HTTP_RESPONSE:
+      case AZ_LOG_HTTP_RETRY:
+      case AZ_LOG_MQTT_RECEIVED_TOPIC:
+      case AZ_LOG_MQTT_RECEIVED_PAYLOAD:
+      case AZ_LOG_IOT_RETRY:
+      case AZ_LOG_IOT_SAS_TOKEN:
+      case AZ_LOG_IOT_AZURERTOS:
+        continue;
+      default:
+        return false;
+    }
+  }
+  return true;
+}
+#endif // AZ_NO_PRECONDITION_CHECKING
+
 void az_log_set_classifications(az_log_classification const classifications[])
 {
+  _az_PRECONDITION(_az_log_classifications_are_valid(classifications));
   _az_log_classifications = classifications;
 }
 

sdk/tests/core/test_az_logging.c

@@ -274,11 +274,113 @@ static void test_az_log_corrupted_response(void** state)
   az_log_set_callback(NULL);
 }
 
+static void _log_listener_no_op(az_log_classification classification, az_span message)
+{
+  assert_true(false);
+  (void)classification;
+  (void)message;
+}
+
+static void test_az_log_incorrect_list_fails_gracefully(void** state)
+{
+  (void)state;
+  {
+    az_log_classification const classifications[] = { AZ_LOG_IOT_RETRY, AZ_LOG_END_OF_LIST };
+    az_log_set_classifications(classifications);
+    az_log_set_callback(_log_listener_no_op);
+
+    assert_false(_az_LOG_SHOULD_WRITE((az_log_classification)12345));
+    _az_LOG_WRITE((az_log_classification)12345, AZ_SPAN_EMPTY);
+
+    az_log_set_callback(NULL);
+    az_log_set_classifications(NULL);
+  }
+}
+
+static int _number_of_log_attempts = 0;
+static void _log_listener_count_logs(az_log_classification classification, az_span message)
+{
+  _number_of_log_attempts++;
+  (void)classification;
+  (void)message;
+}
+
+static void test_az_log_everything_valid(void** state)
+{
+  (void)state;
+  {
+    az_log_classification const classifications[]
+        = { AZ_LOG_IOT_AZURERTOS,         AZ_LOG_IOT_SAS_TOKEN,       AZ_LOG_IOT_RETRY,
+            AZ_LOG_MQTT_RECEIVED_PAYLOAD, AZ_LOG_MQTT_RECEIVED_TOPIC, AZ_LOG_HTTP_RETRY,
+            AZ_LOG_HTTP_RESPONSE,         AZ_LOG_HTTP_REQUEST,        AZ_LOG_END_OF_LIST };
+    az_log_set_classifications(classifications);
+    az_log_set_callback(_log_listener_count_logs);
+
+    _number_of_log_attempts = 0;
+
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_IOT_RETRY));
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_HTTP_REQUEST));
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_IOT_AZURERTOS));
+    assert_false(_az_LOG_SHOULD_WRITE(AZ_LOG_END_OF_LIST));
+    assert_false(_az_LOG_SHOULD_WRITE((az_log_classification)12345));
+    assert_true(
+        _az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_MQTT_RECEIVED_PAYLOAD));
+
+    _az_LOG_WRITE(AZ_LOG_IOT_RETRY, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_HTTP_REQUEST, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_IOT_AZURERTOS, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_END_OF_LIST, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE((az_log_classification)12345, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_MQTT_RECEIVED_PAYLOAD, AZ_SPAN_EMPTY);
+
+    assert_int_equal(_az_BUILT_WITH_LOGGING(4, 0), _number_of_log_attempts);
+
+    az_log_set_callback(NULL);
+    az_log_set_classifications(NULL);
+  }
+}
+
+static void test_az_log_everything_on_null(void** state)
+{
+  (void)state;
+  {
+    az_log_classification const* classifications = NULL;
+    az_log_set_classifications(classifications);
+    az_log_set_callback(_log_listener_count_logs);
+
+    _number_of_log_attempts = 0;
+
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_IOT_RETRY));
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_HTTP_REQUEST));
+    assert_true(_az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_IOT_AZURERTOS));
+    assert_false(_az_LOG_SHOULD_WRITE(AZ_LOG_END_OF_LIST));
+    assert_true(
+        _az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE((az_log_classification)12345));
+    assert_true(
+        _az_BUILT_WITH_LOGGING(true, false) == _az_LOG_SHOULD_WRITE(AZ_LOG_MQTT_RECEIVED_PAYLOAD));
+
+    _az_LOG_WRITE(AZ_LOG_IOT_RETRY, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_HTTP_REQUEST, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_IOT_AZURERTOS, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_END_OF_LIST, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE((az_log_classification)12345, AZ_SPAN_EMPTY);
+    _az_LOG_WRITE(AZ_LOG_MQTT_RECEIVED_PAYLOAD, AZ_SPAN_EMPTY);
+
+    assert_int_equal(_az_BUILT_WITH_LOGGING(5, 0), _number_of_log_attempts);
+
+    az_log_set_callback(NULL);
+    az_log_set_classifications(NULL);
+  }
+}
+
 int test_az_logging()
 {
   const struct CMUnitTest tests[] = {
     cmocka_unit_test(test_az_log),
     cmocka_unit_test(test_az_log_corrupted_response),
+    cmocka_unit_test(test_az_log_incorrect_list_fails_gracefully),
+    cmocka_unit_test(test_az_log_everything_valid),
+    cmocka_unit_test(test_az_log_everything_on_null),
   };
   return cmocka_run_group_tests_name("az_core_logging", tests, NULL, NULL);
 }

sdk/tests/iot/common/test_az_iot_common.c

@@ -250,7 +250,6 @@ static void test_az_iot_provisioning_client_logging_succeed()
   az_log_set_classifications(classifications);
   az_log_set_callback(_log_listener);
 
-  assert_int_equal(0, _log_retry);
   _log_retry = 0;
   assert_int_equal(2229, az_iot_calculate_retry_delay(5, 1, 500, 100000, 1234));
   assert_int_equal(_az_BUILT_WITH_LOGGING(1, 0), _log_retry);