diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ad2d0347..034a679e2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -80,9 +80,12 @@ endif () # Set C specific flags set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}") -set(CMAKE_C_FLAGS "-Wall -Werror -Wno-error=deprecated-declarations ${CMAKE_C_FLAGS}") +set(CMAKE_C_FLAGS "-Wall -Werror -Wformat -Wno-error=deprecated-declarations ${CMAKE_C_FLAGS}") # Set C++ specific flags +set(CMAKE_CXX_FLAGS "-fno-rtti ${CMAKE_CXX_FLAGS}") +set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++ -Wformat -Wno-error=deprecated-declarations ${CMAKE_CXX_FLAGS}") + if (NOT DEFINED CMAKE_CXX_STANDARD) #Celix header only C++ supported is based on C++14 and C++17. #Default using C++14 to ensure the code compiles for C++14. @@ -91,9 +94,6 @@ if (NOT DEFINED CMAKE_CXX_STANDARD) set(CMAKE_CXX_STANDARD 14) endif () -set(CMAKE_CXX_FLAGS "-fno-rtti ${CMAKE_CXX_FLAGS}") -set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++ -Wno-error=deprecated-declarations ${CMAKE_CXX_FLAGS}") - if(APPLE) set(CMAKE_MACOSX_RPATH 1) endif() diff --git a/bundles/deployment_admin/src/deployment_admin.c b/bundles/deployment_admin/src/deployment_admin.c index c471fa9d1..6d513d31d 100644 --- a/bundles/deployment_admin/src/deployment_admin.c +++ b/bundles/deployment_admin/src/deployment_admin.c @@ -16,13 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -/** - * deployment_admin.c - * - * \date Nov 7, 2011 - * \author Apache Celix Project Team - * \copyright Apache License, Version 2.0 - */ #include #include @@ -136,16 +129,6 @@ celix_status_t deploymentAdmin_create(bundle_context_pt context, deployment_admi (*admin)->pollUrl = strdup(pollUrl); (*admin)->auditlogUrl = strdup(auditlogUrl); -// log_store_pt store = NULL; -// log_t *log = NULL; -// log_sync_pt sync = NULL; -// logStore_create(subpool, &store); -// log_create(subpool, store, &log); -// logSync_create(subpool, (*admin)->targetIdentification, store, &sync); -// -// log_log(log, 20000, NULL); - - celixThread_create(&(*admin)->poller, NULL, deploymentAdmin_poll, *admin); } } @@ -199,19 +182,22 @@ static celix_status_t deploymentAdmin_performRequest(deployment_admin_pt admin, fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_ERROR, "Error initializing curl."); } - char url[strlen(admin->auditlogUrl)+6]; - sprintf(url, "%s/send", admin->auditlogUrl); + char* url; + int rc = asprintf(&url, "%s/send", admin->auditlogUrl); + status = rc < 0 ? CELIX_ENOMEM : CELIX_SUCCESS; if (status == CELIX_SUCCESS) { curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1); curl_easy_setopt(curl, CURLOPT_URL, url); curl_easy_setopt(curl, CURLOPT_POSTFIELDS, entry); res = curl_easy_perform(curl); - if (res != CURLE_OK ) { status = CELIX_BUNDLE_EXCEPTION; - fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_ERROR, "Error sending auditlog, got curl error code %d", res); + fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_ERROR, "Error sending auditlog to %s, got curl error code %d", url, res); } + free(url); + } else { + fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_ERROR, "Error creating send url for audit log url: %s", admin->auditlogUrl); } return status; diff --git a/bundles/logging/log_service_api/include/celix_log_service.h b/bundles/logging/log_service_api/include/celix_log_service.h index e766b2676..ff9ada124 100644 --- a/bundles/logging/log_service_api/include/celix_log_service.h +++ b/bundles/logging/log_service_api/include/celix_log_service.h @@ -50,38 +50,38 @@ typedef struct celix_log_service { /** * Logs a trace message, printf style. */ - void (*trace)(void *handle, const char* format, ...); + void (*trace)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a debug message, printf style. */ - void (*debug)(void *handle, const char* format, ...); + void (*debug)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a info message, printf style. */ - void (*info)(void *handle, const char* format, ...); + void (*info)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a warning message, printf style. */ - void (*warning)(void *handle, const char* format, ...); + void (*warning)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a error message, printf style. */ - void (*error)(void *handle, const char* format, ...); + void (*error)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a fatal message, printf style. */ - void (*fatal)(void *handle, const char* format, ...); + void (*fatal)(void *handle, const char* format, ...) __attribute__((format(printf,2,3))); /** * Logs a message using the provided log level, printf style * Silently ignores log level CELIX_LOG_LEVEL_DISABLED. */ - void (*log)(void *handle, celix_log_level_e level, const char* format, ...); + void (*log)(void *handle, celix_log_level_e level, const char* format, ...) __attribute__((format(printf,3,4))); /** * Logs a message using the provided log level, printf style @@ -92,13 +92,22 @@ typedef struct celix_log_service { * * If the argument file or function is NULL, the arguments file, function and line are not used. */ - void (*logDetails)(void *handle, celix_log_level_e level, const char* file, const char* function, int line, const char* format, ...); + void (*logDetails)( + void *handle, celix_log_level_e level, + const char* file, + const char* function, + int line, + const char* format, ...) __attribute__((format(printf,6,7))); /** * Log a message using a format string and va_list argument (vprintf style) * Silently ignores log level CELIX_LOG_LEVEL_DISABLED. */ - void (*vlog)(void *handle, celix_log_level_e level, const char* format, va_list formatArgs); + void (*vlog)( + void *handle, + celix_log_level_e level, + const char* format, + va_list formatArgs) __attribute__((format(printf,3,0))); /** * Log a detailed message using a format string and va_list argument (vprintf style) @@ -109,7 +118,14 @@ typedef struct celix_log_service { * * If the argument file or function is NULL, the arguments file, function and line are not used. */ - void (*vlogDetails)(void *handle, celix_log_level_e level, const char* file, const char* function, int line, const char* format, va_list formatArgs); + void (*vlogDetails)( + void *handle, + celix_log_level_e level, + const char* file, + const char* function, + int line, + const char* format, + va_list formatArgs) __attribute__((format(printf,6,0))); } celix_log_service_t; #ifdef __cplusplus diff --git a/bundles/logging/log_writers/syslog_writer/gtest/src/SyslogWriterTestSuite.cc b/bundles/logging/log_writers/syslog_writer/gtest/src/SyslogWriterTestSuite.cc index 1f73dfe6d..613f8ab6a 100644 --- a/bundles/logging/log_writers/syslog_writer/gtest/src/SyslogWriterTestSuite.cc +++ b/bundles/logging/log_writers/syslog_writer/gtest/src/SyslogWriterTestSuite.cc @@ -91,7 +91,7 @@ TEST_F(SyslogWriterTestSuite, LogToSysLog) { buf[i] = 'A'; } buf[2047] = '\0'; - ls->fatal(ls->handle, buf); + ls->fatal(ls->handle, "%s", buf); }; bool called = celix_bundleContext_useServiceWithOptions(ctx.get(), &opts); EXPECT_TRUE(called); diff --git a/bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c b/bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c index a2193847d..8bf6e1894 100644 --- a/bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c +++ b/bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c @@ -1034,16 +1034,13 @@ int pubsub_tcpHandler_write(pubsub_tcpHandler_t *handle, pubsub_protocol_message message->header.payloadOffset = 0; message->header.isLastSegment = 1; - void *metadataData = NULL; size_t metadataSize = 0; if (message->metadata.metadata) { - metadataSize = entry->writeMetaBufferSize; - metadataData = entry->writeMetaBuffer; + handle->protocol->encodeMetadata(handle->protocol->handle, message, &entry->writeMetaBuffer, &entry->writeMetaBufferSize, &metadataSize); // When maxMsgSize is smaller then meta data is disabled - if (metadataSize > entry->maxMsgSize) { + if (metadataSize > entry->maxMsgSize) { metadataSize = 0; } - handle->protocol->encodeMetadata(handle->protocol->handle, message, &metadataData, &metadataSize); } message->header.metadataSize = metadataSize; @@ -1119,10 +1116,10 @@ int pubsub_tcpHandler_write(pubsub_tcpHandler_t *handle, pubsub_protocol_message // Write optional metadata in vector buffer if (allPayloadAdded && - (metadataSize != 0 && metadataData) && + (metadataSize != 0 && entry->writeMetaBuffer) && (msgPartSize < maxMsgSize) && (msg.msg_iovlen-1 < max_msg_iov_len)) { // header is already included - msg.msg_iov[msg.msg_iovlen].iov_base = metadataData; + msg.msg_iov[msg.msg_iovlen].iov_base = entry->writeMetaBuffer; msg.msg_iov[msg.msg_iovlen].iov_len = metadataSize; msg.msg_iovlen++; msgPartSize += metadataSize; @@ -1202,9 +1199,6 @@ int pubsub_tcpHandler_write(pubsub_tcpHandler_t *handle, pubsub_protocol_message if (payloadData && (payloadData != message->payload.payload)) { free(payloadData); } - if (metadataData && metadataSize > 0) { - free(metadataData); - } } celixThreadMutex_unlock(&entry->writeMutex); } diff --git a/bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_topic_sender.c b/bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_topic_sender.c index 6c82ace7e..3fb68f565 100644 --- a/bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_topic_sender.c +++ b/bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_topic_sender.c @@ -438,9 +438,10 @@ static int psa_zmq_topicPublicationSend(void* handle, unsigned int msgTypeId, co size_t payloadLength = 0; sender->protocol->encodePayload(sender->protocol->handle, &message, &payloadData, &payloadLength); + size_t metadataSize = 0; if (metadata != NULL) { message.metadata.metadata = metadata; - sender->protocol->encodeMetadata(sender->protocol->handle, &message, &sender->zmqBuffers.metadataBuffer, &sender->zmqBuffers.metadataBufferSize); + sender->protocol->encodeMetadata(sender->protocol->handle, &message, &sender->zmqBuffers.metadataBuffer, &sender->zmqBuffers.metadataBufferSize, &metadataSize); } else { message.metadata.metadata = NULL; } @@ -452,7 +453,7 @@ static int psa_zmq_topicPublicationSend(void* handle, unsigned int msgTypeId, co message.header.msgMajorVersion = majorVersion; message.header.msgMinorVersion = minorversion; message.header.payloadSize = payloadLength; - message.header.metadataSize = sender->zmqBuffers.metadataBufferSize; + message.header.metadataSize = metadataSize; message.header.payloadPartSize = payloadLength; message.header.payloadOffset = 0; message.header.isLastSegment = 1; @@ -484,7 +485,7 @@ static int psa_zmq_topicPublicationSend(void* handle, unsigned int msgTypeId, co //send Payload if (rc > 0) { - int flag = ((sender->zmqBuffers.metadataBufferSize > 0) || (sender->zmqBuffers.footerBufferSize > 0)) ? ZMQ_SNDMORE : 0; + int flag = ((metadataSize > 0) || (sender->zmqBuffers.footerBufferSize > 0)) ? ZMQ_SNDMORE : 0; zmq_msg_init_data(&msg2, payloadData, payloadLength, psa_zmq_freeMsg, freeMsgEntry); rc = zmq_msg_send(&msg2, socket, flag); if (rc == -1) { @@ -494,9 +495,9 @@ static int psa_zmq_topicPublicationSend(void* handle, unsigned int msgTypeId, co } //send MetaData - if (rc > 0 && sender->zmqBuffers.metadataBufferSize > 0) { + if (rc > 0 && metadataSize > 0) { int flag = (sender->zmqBuffers.footerBufferSize > 0 ) ? ZMQ_SNDMORE : 0; - zmq_msg_init_data(&msg3, sender->zmqBuffers.metadataBuffer, sender->zmqBuffers.metadataBufferSize, NULL, NULL); + zmq_msg_init_data(&msg3, sender->zmqBuffers.metadataBuffer, metadataSize, NULL, NULL); rc = zmq_msg_send(&msg3, socket, flag); if (rc == -1) { L_WARN("Error sending metadata msg. %s", strerror(errno)); @@ -519,8 +520,8 @@ static int psa_zmq_topicPublicationSend(void* handle, unsigned int msgTypeId, co zmsg_t *msg = zmsg_new(); zmsg_addmem(msg, sender->zmqBuffers.headerBuffer, sender->zmqBuffers.headerBufferSize); zmsg_addmem(msg, payloadData, payloadLength); - if (sender->zmqBuffers.metadataBufferSize > 0) { - zmsg_addmem(msg, sender->zmqBuffers.metadataBuffer, sender->zmqBuffers.metadataBufferSize); + if (metadataSize > 0) { + zmsg_addmem(msg, sender->zmqBuffers.metadataBuffer, metadataSize); } if (sender->zmqBuffers.footerBufferSize > 0) { zmsg_addmem(msg, sender->zmqBuffers.footerBuffer, sender->zmqBuffers.footerBufferSize); diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/CMakeLists.txt b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/CMakeLists.txt index 460ab79fe..d21d3c90f 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/CMakeLists.txt +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/CMakeLists.txt @@ -15,14 +15,12 @@ # specific language governing permissions and limitations # under the License. -set(SOURCES - src/main.cc - src/PS_WP_common_tests.cc - ) -add_executable(celix_pswp_common_tests ${SOURCES}) -#target_include_directories(celix_cxx_pswp_tests SYSTEM PRIVATE gtest) +add_executable(celix_pswp_common_tests src/PS_WP_common_tests.cc) target_include_directories(celix_pswp_common_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/../src) -target_link_libraries(celix_pswp_common_tests PRIVATE celix_pubsub_protocol_lib GTest::gtest Celix::pubsub_spi) +target_link_libraries(celix_pswp_common_tests PRIVATE celix_pubsub_protocol_lib GTest::gtest Celix::pubsub_spi GTest::gtest_main ${CMAKE_DL_LIBS}) +if (NOT ENABLE_ADDRESS_SANITIZER) + target_compile_definitions(celix_pswp_common_tests PRIVATE ENABLE_MALLOC_RETURN_NULL_TESTS) +endif () add_test(NAME celix_pswp_common_tests COMMAND celix_pswp_common_tests) setup_target_for_coverage(celix_pswp_common_tests SCAN_DIR ..) \ No newline at end of file diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/PS_WP_common_tests.cc b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/PS_WP_common_tests.cc index 4d03132dc..68fc3bfa5 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/PS_WP_common_tests.cc +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/PS_WP_common_tests.cc @@ -17,14 +17,14 @@ *under the License. */ -#include - -#include -#include "gtest/gtest.h" +#include +#include #include -#include +#include + +#include "pubsub_wire_protocol_common.h" class WireProtocolCommonTest : public ::testing::Test { public: @@ -32,26 +32,270 @@ class WireProtocolCommonTest : public ::testing::Test { ~WireProtocolCommonTest() override = default; }; -TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeMetadata_Benchmark) { // NOLINT(cert-err58-cpp) +#ifdef ENABLE_MALLOC_RETURN_NULL_TESTS +/** + * If set to true the mocked malloc will always return NULL. + * Should be read/written using __atomic builtins. + */ +static int mallocFailAfterCalls = 0; +static int mallocCurrentCallCount = 0; + +/** + * mocked malloc to ensure testing can be done for the "malloc returns NULL" scenario + */ +extern "C" void* malloc(size_t size) { + int count = __atomic_add_fetch(&mallocCurrentCallCount, 1, __ATOMIC_ACQ_REL); + int target = __atomic_load_n(&mallocFailAfterCalls, __ATOMIC_ACQUIRE); + if (target > 0 && count == target) { + return nullptr; + } + static auto* orgMallocFp = (void*(*)(size_t))dlsym(RTLD_NEXT, "malloc"); + if (orgMallocFp == nullptr) { + perror("Cannot find malloc symbol"); + return nullptr; + } + return orgMallocFp(size); +} + +extern "C" void* realloc(void* buf, size_t newSize) { + int count = __atomic_add_fetch(&mallocCurrentCallCount, 1, __ATOMIC_ACQ_REL); + int target = __atomic_load_n(&mallocFailAfterCalls, __ATOMIC_ACQUIRE); + if (target > 0 && count == target) { + return nullptr; + } + static auto* orgReallocFp = (void*(*)(void*, size_t))dlsym(RTLD_NEXT, "realloc"); + if (orgReallocFp == nullptr) { + perror("Cannot find realloc symbol"); + return nullptr; + } + return orgReallocFp(buf, newSize); +} + +void setupMallocFailAfterNrCalls(int nrOfCalls) { + __atomic_store_n(&mallocFailAfterCalls, nrOfCalls, __ATOMIC_RELEASE); + __atomic_store_n(&mallocCurrentCallCount, 0, __ATOMIC_RELEASE); +} + +void disableMallocFail() { + __atomic_store_n(&mallocFailAfterCalls, 0, __ATOMIC_RELEASE); +} +#endif + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeMetadataWithSingleEntries) { pubsub_protocol_message_t message; message.header.convertEndianess = 0; message.metadata.metadata = celix_properties_create(); celix_properties_set(message.metadata.metadata, "key1", "value1"); - celix_properties_set(message.metadata.metadata, "key2", "value2"); - celix_properties_set(message.metadata.metadata, "key3", "value3"); - celix_properties_set(message.metadata.metadata, "key4", "value4"); - celix_properties_set(message.metadata.metadata, "key5", "value5"); + size_t neededNetstringLength = 16; //4:key1,6:value1, (16 chars) + + char *data = nullptr; + size_t length = 0; + size_t contentLength = 0; + celix_status_t status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + EXPECT_TRUE(status == CELIX_SUCCESS); + EXPECT_EQ(contentLength, neededNetstringLength + 4); + + //note first 4 bytes are an int with the nr of metadata entries (6); + uint32_t nrOfMetadataEntries; + pubsubProtocol_readInt((unsigned char*)data, 0, message.header.convertEndianess, &nrOfMetadataEntries); + EXPECT_EQ(1, nrOfMetadataEntries); + EXPECT_GE(length, neededNetstringLength + 4 /*sizeof(nrOfMetadataEntries)*/); + + //create null terminated string from netstring entries in the data buffer (- first 4 bytes + null terminating entry) + char checkStr[32]; + ASSERT_GE(sizeof(checkStr), neededNetstringLength); + strncpy(checkStr, data+4, neededNetstringLength); + checkStr[neededNetstringLength] = '\0'; + EXPECT_STREQ("4:key1,6:value1,", checkStr); + + free(data); + celix_properties_destroy(message.metadata.metadata); +} + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeMetadataWithMultipleEntries) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 1; + message.metadata.metadata = celix_properties_create(); + celix_properties_set(message.metadata.metadata, "key1", "value1"); //4:key1,6:value1, (16 chars) + celix_properties_set(message.metadata.metadata, "key2", "value2"); //4:key2,6:value2, (16 chars) + celix_properties_set(message.metadata.metadata, "key3", "value3"); //4:key3,6:value3, (16 chars) + celix_properties_set(message.metadata.metadata, "key4", "value4"); //4:key4,6:value4, (16 chars) + celix_properties_set(message.metadata.metadata, "key5", "value5"); //4:key5,6:value5, (16 chars) + celix_properties_set(message.metadata.metadata, "key111", "value111"); //6:key111,8:value111, (20 chars) + size_t neededNetstringLength = 100; //Total: 5 * 16 + 20 = 100 + + + char *data = nullptr; + size_t length = 0; + size_t contentLength = 0; + celix_status_t status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + EXPECT_TRUE(status == CELIX_SUCCESS); + EXPECT_EQ(contentLength, neededNetstringLength + 4); + + //note first 4 bytes are a int with the nr of metadata entries (6); + uint32_t nrOfMetadataEntries; + pubsubProtocol_readInt((unsigned char*)data, 0, message.header.convertEndianess, &nrOfMetadataEntries); + EXPECT_EQ(6, nrOfMetadataEntries); + + + //create null terminated string from netstring entries in the data buffer (- first 4 bytes + null terminating entry) + char checkStr[128]; + ASSERT_GE(sizeof(checkStr), neededNetstringLength); + strncpy(checkStr, data+4, neededNetstringLength); + checkStr[neededNetstringLength] = '\0'; - void *data = nullptr; + //NOTE because celix properties can reorder entries (hashmap), check if the expected str parts are in the data. + EXPECT_NE(nullptr, strstr(checkStr, "4:key1,6:value1,")); //entry 1 + EXPECT_NE(nullptr, strstr(checkStr, "4:key2,6:value2,")); //entry 2 + EXPECT_NE(nullptr, strstr(checkStr, "4:key3,6:value3,")); //entry 3 + EXPECT_NE(nullptr, strstr(checkStr, "4:key4,6:value4,")); //entry 4 + EXPECT_NE(nullptr, strstr(checkStr, "4:key5,6:value5,")); //entry 5 + EXPECT_NE(nullptr, strstr(checkStr, "6:key111,8:value111,")); //entry 6 (with value 111) + + free(data); + celix_properties_destroy(message.metadata.metadata); +} + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeMetadataWithNullOrEmptyArguments) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 0; + message.metadata.metadata = nullptr; + + char *data = nullptr; size_t length = 0; - auto start = std::chrono::system_clock::now(); - for(int i = 0; i < 100000; i++) { - celix_status_t status = pubsubProtocol_encodeMetadata(&message, &data, &length); - ASSERT_TRUE(status == CELIX_SUCCESS); + size_t contentLength = 0; + celix_status_t status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(contentLength, 4); + + //note first 4 bytes are a int with the nr of metadata entries (0); + uint32_t nrOfMetadataEntries; + pubsubProtocol_readInt((unsigned char*)data, 0, message.header.convertEndianess, &nrOfMetadataEntries); + EXPECT_EQ(0, nrOfMetadataEntries); + free(data); + + data = nullptr; + length = 0; + message.metadata.metadata = celix_properties_create(); //note empty + + status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(contentLength, 4); + + //note first 4 bytes are a int with the nr of metadata entries (0); + pubsubProtocol_readInt((unsigned char*)data, 0, message.header.convertEndianess, &nrOfMetadataEntries); + EXPECT_EQ(0, nrOfMetadataEntries); + free(data); + celix_properties_destroy(message.metadata.metadata); +} + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeWithExistinBufferWhichAlsoNeedsSomeRealloc) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 0; + message.metadata.metadata = nullptr; + + char *data = (char*)malloc(2); + size_t length = 2; + size_t contentLength = 0; + celix_status_t status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_GT(length, 2); + + message.metadata.metadata = celix_properties_create(); + char key[32]; + //create a lot of metadata entries + for (int i = 0; i < 1000; ++i) { + snprintf(key, sizeof(key), "key%i", i); + celix_properties_set(message.metadata.metadata, key, "abcdefghijklmnop"); } - auto end = std::chrono::system_clock::now(); + //note reusing data + status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + EXPECT_EQ(status, CELIX_SUCCESS); + free(data); + celix_properties_destroy(message.metadata.metadata); +} + +#ifdef ENABLE_MALLOC_RETURN_NULL_TESTS +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_EncodeMetadataWithNoMemoryLeft) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 0; + message.metadata.metadata = celix_properties_create(); + celix_properties_set(message.metadata.metadata, "key1", "value1"); + + //Scenario: No mem with no pre-allocated data + //Given (mocked) malloc is forced to return NULL + setupMallocFailAfterNrCalls(1); + + //When I try to encode a metadata + char *data = nullptr; + size_t length = 0; + size_t contentLength = 0; + auto status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + //Then I expect a failure + EXPECT_NE(status, CELIX_SUCCESS); + + //reset malloc + disableMallocFail(); + + //Scenario: No mem with some pre-allocated data + //Given a data set with some space + data = (char*)malloc(16); + length = 16; - std::cout << "WireProtocolCommonTest_EncodeMetadata_Benchmark took " << std::chrono::duration_cast(end-start).count() << " µs\n"; + //And (mocked) malloc is forced to return NULL + setupMallocFailAfterNrCalls(1); + + //When I try to encode a metadata + status = pubsubProtocol_encodeMetadata(&message, &data, &length, &contentLength); + + //Then I expect a failure + EXPECT_NE(status, CELIX_SUCCESS); + + //reset malloc + disableMallocFail(); + + free(data); celix_properties_destroy(message.metadata.metadata); -} \ No newline at end of file +} +#endif + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_DecodeMetadataWithSingleEntries) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 0; + message.metadata.metadata = nullptr; + + char* data = strdup("ABCD4:key1,6:value1,"); //note 1 entry + auto len = strlen(data); + pubsubProtocol_writeInt((unsigned char*)data, 0, message.header.convertEndianess, 1); + auto status = pubsubProtocol_decodeMetadata((void*)data, len, &message); + + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(1, celix_properties_size(message.metadata.metadata)); + EXPECT_STREQ("value1", celix_properties_get(message.metadata.metadata, "key1", "not-found")); + + free(data); + celix_properties_destroy(message.metadata.metadata); +} + +TEST_F(WireProtocolCommonTest, WireProtocolCommonTest_DencodeMetadataWithMultipleEntries) { + pubsub_protocol_message_t message; + message.header.convertEndianess = 1; + message.metadata.metadata = nullptr; + + char* data = strdup("ABCD4:key1,6:value1,4:key2,6:value2,6:key111,8:value111,"); //note 3 entries + auto len = strlen(data); + pubsubProtocol_writeInt((unsigned char*)data, 0, message.header.convertEndianess, 3); + auto status = pubsubProtocol_decodeMetadata((void*)data, len, &message); + + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(3, celix_properties_size(message.metadata.metadata)); + EXPECT_STREQ("value1", celix_properties_get(message.metadata.metadata, "key1", "not-found")); + EXPECT_STREQ("value2", celix_properties_get(message.metadata.metadata, "key2", "not-found")); + EXPECT_STREQ("value111", celix_properties_get(message.metadata.metadata, "key111", "not-found")); + + free(data); + celix_properties_destroy(message.metadata.metadata); +} + diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/main.cc b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/main.cc deleted file mode 100644 index 09731c411..000000000 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/gtest/src/main.cc +++ /dev/null @@ -1,26 +0,0 @@ -/** - *Licensed to the Apache Software Foundation (ASF) under one - *or more contributor license agreements. See the NOTICE file - *distributed with this work for additional information - *regarding copyright ownership. The ASF licenses this file - *to you under the Apache License, Version 2.0 (the - *"License"); you may not use this file except in compliance - *with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - *Unless required by applicable law or agreed to in writing, - *software distributed under the License is distributed on an - *"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - *specific language governing permissions and limitations - *under the License. - */ - -#include - -int main(int argc, char **argv) { - ::testing::InitGoogleTest(&argc, argv); - int rc = RUN_ALL_TESTS(); - return rc; -} \ No newline at end of file diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/include/pubsub_wire_protocol_common.h b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/include/pubsub_wire_protocol_common.h index 7dd1ef2e9..f6f1b8abc 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/include/pubsub_wire_protocol_common.h +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/include/pubsub_wire_protocol_common.h @@ -54,10 +54,30 @@ int pubsubProtocol_writeInt(unsigned char *data, int offset, uint32_t convert, u int pubsubProtocol_writeLong(unsigned char *data, int offset, uint32_t convert, uint64_t val); celix_status_t pubsubProtocol_encodePayload(pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); -celix_status_t pubsubProtocol_encodeMetadata(pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); celix_status_t pubsubProtocol_decodePayload(void *data, size_t length, pubsub_protocol_message_t *message); celix_status_t pubsubProtocol_decodeMetadata(void *data, size_t length, pubsub_protocol_message_t *message); +/** + * @brief Encode metadata to bufferInOut. + * + * The metadata is encoded as a bytebuffer where the first 4 bytes is used to specify how many metadata (key,value) + * entries there are and then every metadata entry is encoded as a netstring key followed by a netstring value. + * + * If *bufferInOut is NULL, a new buffer will be allocated. If *bufferInOut is not NULL, the buffer is reused and the + * provided *bufferLengthInOut must indicate the length of the provided buffer. + * If a provided *bufferInOut is not large enough to fit the encoded metadata, the buffer will be reallocated. + * + * If this calls return with an error, the caller is still owner of a possible returned output buffer. + * + * @param message The message containing the metadata to encode + * @param bufferInOut Input/output argument for the buffer, if call is successful will contain the metadata header + * @param bufferLengthInOut Input/output arguments for the length of the bufferInOut argument. + * @param bufferContentLengthOut Output argument for the actual content size of the bufferInOut. Note that the + * bufferContentLengthOut can be smaller than the buffer length. + * @return CELIX_SUCCESS if encoding was successful. + */ +celix_status_t pubsubProtocol_encodeMetadata(pubsub_protocol_message_t* message, char** bufferInOut, size_t* bufferLengthInOut, size_t* bufferContentLengthOut); + #ifdef __cplusplus } diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c index 1d123d7b7..eb867ab5e 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c @@ -21,52 +21,114 @@ #include #include -#include #include #include "celix_byteswap.h" +#include "celix_utils.h" -static celix_status_t pubsubProtocol_createNetstring(const char* string, char** netstringOut, int *netstringOutLength, int *netstringMemoryOutLength) { - celix_status_t status = CELIX_SUCCESS; +#define PUBSUB_METADATA_INITIAL_BUFFER_SIZE 1024 +#define PUBSUB_METADATA_MAX_BUFFER_SIZE (1024 * 1024 * 1024) //max 1gb +#define PUBSUB_METADATA_BUFFER_INCREASE_FACTOR 2.0 - size_t str_len = strlen(string); - if (str_len == 0) { - // 0:, - if(*netstringOut == NULL) { - *netstringMemoryOutLength = 1024; - *netstringOut = calloc(1, 1024); - *netstringOutLength = 3; - } - if (*netstringOut == NULL) { - status = CELIX_ENOMEM; +/** + * @brief Add - as netstring formatted - str to the provided buffer. + * + * The provided offset will be updated to point to the next unused byte for the provided buffer. + * + * @return CELIX_FILE_IO_EXCEPTION if the provided buffer is not big enough. + */ +static celix_status_t pubsubProtocol_addNetstringEntryToBuffer(char* buffer, size_t bufferSize, size_t* offsetInOut, const char* str) { + size_t offset = *offsetInOut; + + size_t strLen = strnlen(str, CELIX_UTILS_MAX_STRLEN); + if (strLen == CELIX_UTILS_MAX_STRLEN) { + return CELIX_ILLEGAL_ARGUMENT; + } + + char strLenString[32]; //note the str needed to print the strLen of str. + int written = snprintf(strLenString, sizeof(strLenString), "%zu", strLen); + if (written >= sizeof(strLenString) || written < 0) { + return CELIX_ILLEGAL_ARGUMENT; //str too large to capture in 31 characters. + } + size_t strLenStringLen = written; + + if (strLen == 0) { + if ((bufferSize - offset) < 3) { + return CELIX_FILE_IO_EXCEPTION; } else { - *netstringOut[0] = '0'; - *netstringOut[1] = ':'; - *netstringOut[2] = ','; - *netstringOut[3] = '\0'; + memcpy(buffer + offset, "0:,", 3); + offset += 3; } } else { - size_t numlen = ceil(log10(str_len + 1)); - if(*netstringOut == NULL) { - *netstringMemoryOutLength = 1024; - *netstringOut = calloc(1, *netstringMemoryOutLength); - if (*netstringOut == NULL) { + size_t sizeNeeded = strLenStringLen + 1 /*:*/ + strLen + 1 /*,*/; //e.g "5:hello," + if (sizeNeeded > bufferSize - offset) { + return CELIX_FILE_IO_EXCEPTION; + } + memcpy(buffer + offset, strLenString, strLenStringLen); //e.g "5" + offset += strLenStringLen; + buffer[offset++] = ':'; + memcpy(buffer + offset, str, strLen); //e.g. "hello" + offset += strLen; + buffer[offset++] = ','; + } + + *offsetInOut = offset; + return CELIX_SUCCESS; +} + +celix_status_t pubsubProtocol_encodeMetadata(pubsub_protocol_message_t* message, char** bufferInOut, size_t* bufferLengthInOut, size_t* bufferContentLengthOut) { + int metadataSize = message->metadata.metadata == NULL ? 0 : celix_properties_size(message->metadata.metadata); + bool reallocBuffer = false; + bool encoded = false; + + if (*bufferInOut == NULL || *bufferLengthInOut < 4 /*note buffer needs to fit int for nr of metadata entries*/) { + reallocBuffer = true; + } + + size_t offset = 4; //note starting at index 4, to keep the first 4 bytes free for the nr metadata elements entry + while (!encoded) { + if (reallocBuffer) { + size_t newLength = *bufferInOut == NULL ? + PUBSUB_METADATA_INITIAL_BUFFER_SIZE : + (size_t)((double)*bufferLengthInOut * PUBSUB_METADATA_BUFFER_INCREASE_FACTOR); + if (newLength > PUBSUB_METADATA_MAX_BUFFER_SIZE) { + //max buffer size reached + return CELIX_ILLEGAL_ARGUMENT; + } + char* newBuffer = realloc(*bufferInOut, newLength); + if (newBuffer == NULL) { return CELIX_ENOMEM; } - } else if (*netstringMemoryOutLength < numlen + str_len + 2) { - free(*netstringOut); - while(*netstringMemoryOutLength < numlen + str_len + 2) { - *netstringMemoryOutLength *= 2; + *bufferInOut = newBuffer; + *bufferLengthInOut = newLength; + } + const char* key; + if (metadataSize == 0) { + encoded = true; + continue; + } + celix_status_t status = CELIX_SUCCESS; + CELIX_PROPERTIES_FOR_EACH(message->metadata.metadata, key) { + const char *val = celix_properties_get(message->metadata.metadata, key, NULL); + if (status == CELIX_SUCCESS) { + status = pubsubProtocol_addNetstringEntryToBuffer(*bufferInOut, *bufferLengthInOut, &offset, key); } - *netstringOut = calloc(1, *netstringMemoryOutLength); - if (*netstringOut == NULL) { - return CELIX_ENOMEM; + if (status == CELIX_SUCCESS) { + status = pubsubProtocol_addNetstringEntryToBuffer(*bufferInOut, *bufferLengthInOut, &offset, val); } } - *netstringOutLength = numlen + str_len + 2; - sprintf(*netstringOut, "%zu:%s,", str_len, string); + if (status == CELIX_FILE_IO_EXCEPTION) { + reallocBuffer = true; + continue; + } else if (status != CELIX_SUCCESS) { + return status; + } + encoded =true; } - return status; + //write nr of meta elements at the beginning of the pubsub protocol metadata entry + pubsubProtocol_writeInt((unsigned char *)*bufferInOut, 0, message->header.convertEndianess, metadataSize); + *bufferContentLengthOut = offset; + return CELIX_SUCCESS; } /* Reads a netstring from a `buffer` of length `buffer_length`. Writes @@ -190,76 +252,6 @@ celix_status_t pubsubProtocol_encodePayload(pubsub_protocol_message_t *message, return CELIX_SUCCESS; } -celix_status_t pubsubProtocol_encodeMetadata(pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength) { - celix_status_t status = CELIX_SUCCESS; - - size_t lineMemoryLength = *outBuffer == NULL ? 1024 : *outLength; - unsigned char *line = *outBuffer == NULL ? calloc(1, lineMemoryLength) : *outBuffer; - size_t idx = 4; - size_t len = 0; - - if (message->metadata.metadata != NULL && celix_properties_size(message->metadata.metadata) > 0) { - const char *key; - char *keyNetString = NULL; - int netStringMemoryLength = 0; - - CELIX_PROPERTIES_FOR_EACH(message->metadata.metadata, key) { - const char *val = celix_properties_get(message->metadata.metadata, key, "!Error!"); - - //refactoring these two copies to a function leads to a slow down of about 2x - int strlenKeyNetString = 0; - status = pubsubProtocol_createNetstring(key, &keyNetString, &strlenKeyNetString, &netStringMemoryLength); - if (status != CELIX_SUCCESS) { - break; - } - - len += strlenKeyNetString; - if(lineMemoryLength < len + 1) { - lineMemoryLength *= 2; - unsigned char *tmp = realloc(line, lineMemoryLength); - if (!tmp) { - free(line); - status = CELIX_ENOMEM; - break; - } - line = tmp; - } - memcpy(line + idx, keyNetString, strlenKeyNetString); - idx += strlenKeyNetString; - - status = pubsubProtocol_createNetstring(val, &keyNetString, &strlenKeyNetString, &netStringMemoryLength); - if (status != CELIX_SUCCESS) { - break; - } - - len += strlenKeyNetString; - if(lineMemoryLength < len + 1) { - while(lineMemoryLength < len + 1) { - lineMemoryLength *= 2; - } - unsigned char *tmp = realloc(line, lineMemoryLength); - if (!tmp) { - free(line); - status = CELIX_ENOMEM; - break; - } - line = tmp; - } - memcpy(line + idx, keyNetString, strlenKeyNetString); - idx += strlenKeyNetString; - } - - free(keyNetString); - } - int size = celix_properties_size(message->metadata.metadata); - pubsubProtocol_writeInt((unsigned char *) line, 0, true, size); - - *outBuffer = line; - *outLength = idx; - - return status; -} - celix_status_t pubsubProtocol_decodePayload(void *data, size_t length, pubsub_protocol_message_t *message){ message->payload.payload = data; message->payload.length = length; @@ -271,7 +263,7 @@ celix_status_t pubsubProtocol_decodeMetadata(void *data, size_t length, pubsub_p celix_status_t status = CELIX_SUCCESS; uint32_t nOfElements; - size_t idx = pubsubProtocol_readInt(data, 0, true, &nOfElements); + size_t idx = pubsubProtocol_readInt(data, 0, message->header.convertEndianess, &nOfElements); unsigned char *netstring = data + idx; int netstringLen = length - idx; diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/gtest/src/PS_WP_tests.cc b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/gtest/src/PS_WP_tests.cc index c39604a23..1e5f03ea9 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/gtest/src/PS_WP_tests.cc +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/gtest/src/PS_WP_tests.cc @@ -74,6 +74,38 @@ TEST_F(WireProtocolV1Test, WireProtocolV1Test_EncodeHeader_Test) { // NOLINT(cer free(headerData); } +TEST_F(WireProtocolV1Test, WireProtocolV1Test_EncodeHeader_TestWithExistingMemory) { + pubsub_protocol_wire_v1_t *wireprotocol; + pubsubProtocol_create(&wireprotocol); + + pubsub_protocol_message_t message; + message.header.msgId = 1; + message.header.msgMajorVersion = 0; + message.header.msgMinorVersion = 0; + message.header.payloadSize = 2; + message.header.metadataSize = 3; + + void* headerData = malloc(3); + size_t headerLength = 0; + + //calling with too small of a buffer (new buffer with new length should be created). + celix_status_t status = pubsubProtocol_encodeHeader(nullptr, &message, &headerData, &headerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(24, headerLength); + EXPECT_NE(3, headerLength); + + void* orgHeaderDataPointer = headerData; + //calling with matching buffer, buffer will be re-used. + status = pubsubProtocol_encodeHeader(nullptr, &message, &headerData, &headerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(24, headerLength); + EXPECT_EQ(headerData, orgHeaderDataPointer); + + + pubsubProtocol_destroy(wireprotocol); + free(headerData); +} + TEST_F(WireProtocolV1Test, WireProtocolV1Test_DecodeHeader_Test) { // NOLINT(cert-err58-cpp) pubsub_protocol_wire_v1_t *wireprotocol; pubsubProtocol_create(&wireprotocol); @@ -167,11 +199,14 @@ TEST_F(WireProtocolV1Test, WireProtocolV1Test_EncodeMetadata_Test) { // NOLINT(c pubsub_protocol_message_t message; message.metadata.metadata = celix_properties_create(); + message.header.convertEndianess = 1; celix_properties_set(message.metadata.metadata, "a", "b"); void *data = nullptr; + size_t dataLength = 0; size_t length = 0; - celix_status_t status = pubsubProtocol_v1_encodeMetadata(nullptr, &message, &data, &length); + celix_status_t status = pubsubProtocol_v1_encodeMetadata(nullptr, &message, &data, &dataLength, &length); + ASSERT_EQ(status, CELIX_SUCCESS); unsigned char exp[12]; uint32_t s = htonl(1); @@ -181,7 +216,7 @@ TEST_F(WireProtocolV1Test, WireProtocolV1Test_EncodeMetadata_Test) { // NOLINT(c ASSERT_EQ(status, CELIX_SUCCESS); ASSERT_EQ(12, length); for (int i = 0; i < 12; i++) { - ASSERT_EQ(((unsigned char*) data)[i], exp[i]); + ASSERT_EQ(((unsigned char*) data)[i], exp[i]) << "Error at index " << i; } celix_properties_destroy(message.metadata.metadata); diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.c b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.c index c6ce26e3b..04048a2ae 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.c +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.c @@ -87,7 +87,8 @@ celix_status_t pubsubProtocol_encodeHeader(void *handle, pubsub_protocol_message size_t headerSize = 0; pubsubProtocol_getHeaderSize(handle, &headerSize); - if (*outBuffer == NULL) { + if (*outBuffer == NULL || headerSize != *outLength) { + free(*outBuffer); *outBuffer = calloc(1, headerSize); *outLength = headerSize; } @@ -113,12 +114,11 @@ celix_status_t pubsubProtocol_v1_encodePayload(void *handle __attribute__((unuse return pubsubProtocol_encodePayload(message, outBuffer, outLength); } -celix_status_t pubsubProtocol_v1_encodeMetadata(void *handle __attribute__((unused)), pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength) { - return pubsubProtocol_encodeMetadata(message, outBuffer, outLength); +celix_status_t pubsubProtocol_v1_encodeMetadata(void *handle __attribute__((unused)), pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut, size_t *bufferContentLengthOut) { + return pubsubProtocol_encodeMetadata(message, (char**)bufferInOut, bufferLengthInOut, bufferContentLengthOut); } celix_status_t pubsubProtocol_encodeFooter(void *handle __attribute__((unused)), pubsub_protocol_message_t *message __attribute__((unused)), void **outBuffer, size_t *outLength) { - *outBuffer = NULL; return pubsubProtocol_getFooterSize(handle, outLength); } diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.h b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.h index 9b7521faa..6e8ef9246 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.h +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v1/src/pubsub_wire_protocol_impl.h @@ -42,7 +42,7 @@ celix_status_t pubsubProtocol_isMessageSegmentationSupported(void* handle, bool celix_status_t pubsubProtocol_encodeHeader(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); celix_status_t pubsubProtocol_v1_encodePayload(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); -celix_status_t pubsubProtocol_v1_encodeMetadata(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); +celix_status_t pubsubProtocol_v1_encodeMetadata(void *handle __attribute__((unused)), pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut, size_t *bufferContentLengthOut); celix_status_t pubsubProtocol_encodeFooter(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); celix_status_t pubsubProtocol_decodeHeader(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/gtest/src/PS_WP_v2_tests.cc b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/gtest/src/PS_WP_v2_tests.cc index 080b6f032..3a1dbda0f 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/gtest/src/PS_WP_v2_tests.cc +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/gtest/src/PS_WP_v2_tests.cc @@ -89,6 +89,41 @@ TEST_F(WireProtocolV2Test, WireProtocolV2Test_EncodeHeader_Test) { // NOLINT(cer free(headerData); } +TEST_F(WireProtocolV2Test, WireProtocolV2Test_EncodeHeader_TestWithExistingBuffer) { + pubsub_protocol_wire_v2_t *wireprotocol; + pubsubProtocol_wire_v2_create(&wireprotocol); + + pubsub_protocol_message_t message; + message.header.msgId = 1; + message.header.seqNr = 4; + message.header.msgMajorVersion = 0; + message.header.msgMinorVersion = 0; + message.header.payloadSize = 2; + message.header.metadataSize = 3; + message.header.payloadPartSize = 4; + message.header.payloadOffset = 2; + message.header.isLastSegment = 1; + message.header.convertEndianess = 1; + + void *headerData = malloc(3); + size_t headerLength = 3; + + //calling with too small of a buffer (new buffer with new length should be created). + celix_status_t status = pubsubProtocol_wire_v2_encodeHeader(nullptr, &message, &headerData, &headerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(40, headerLength); + + void* orgHeaderDataPointer = headerData; + //calling with matching buffer, buffer will be re-used. + status = pubsubProtocol_wire_v2_encodeHeader(nullptr, &message, &headerData, &headerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(40, headerLength); + EXPECT_EQ(headerData, orgHeaderDataPointer); + + pubsubProtocol_wire_v2_destroy(wireprotocol); + free(headerData); +} + TEST_F(WireProtocolV2Test, WireProtocolV2Test_DecodeHeader_Test) { // NOLINT(cert-err58-cpp) pubsub_protocol_wire_v2_t *wireprotocol; pubsubProtocol_wire_v2_create(&wireprotocol); @@ -218,6 +253,31 @@ TEST_F(WireProtocolV2Test, WireProtocolV2Test_EncodeFooter_Test) { // NOLINT(cer free(footerData); } +TEST_F(WireProtocolV2Test, WireProtocolV2Test_EncodeFooter_TestWithExistingBuffer) { + pubsub_protocol_wire_v2_t *wireprotocol; + pubsubProtocol_wire_v2_create(&wireprotocol); + + pubsub_protocol_message_t message; + message.header.convertEndianess = 0; + + void *footerData = malloc(3); + size_t footerLength = 3; + //calling with too small of a buffer (new buffer with new length should be created). + celix_status_t status = pubsubProtocol_wire_v2_encodeFooter(nullptr, &message, &footerData, &footerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_NE(3, footerLength); + + + void* orgFooterDataPointer = footerData; + //calling with matching buffer, buffer will be re-used. + status = pubsubProtocol_wire_v2_encodeFooter(nullptr, &message, &footerData, &footerLength); + EXPECT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(footerData, orgFooterDataPointer); + + pubsubProtocol_wire_v2_destroy(wireprotocol); + free(footerData); +} + TEST_F(WireProtocolV2Test, WireProtocolV2Test_DecodeFooter_Test) { // NOLINT(cert-err58-cpp) pubsub_protocol_wire_v2_t *wireprotocol; pubsubProtocol_wire_v2_create(&wireprotocol); diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.c b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.c index 86a46bd6b..ff51c01d7 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.c +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.c @@ -80,7 +80,9 @@ celix_status_t pubsubProtocol_wire_v2_encodeHeader(void *handle, pubsub_protocol size_t headerSize = 0; pubsubProtocol_wire_v2_getHeaderSize(handle, &headerSize); - if (*outBuffer == NULL) { + if (*outBuffer == NULL || *outLength != headerSize) { + //allocated or reallocate memory for header + free(*outBuffer); *outBuffer = malloc(headerSize); *outLength = headerSize; } @@ -112,7 +114,9 @@ celix_status_t pubsubProtocol_wire_v2_encodeFooter(void *handle, pubsub_protocol size_t footerSize = 0; pubsubProtocol_wire_v2_getFooterSize(handle, &footerSize); - if (*outBuffer == NULL) { + if (*outBuffer == NULL || *outLength != footerSize) { + //allocated or reallocate memory for footer + free(*outBuffer); *outBuffer = malloc(footerSize); *outLength = footerSize; } @@ -192,8 +196,8 @@ celix_status_t pubsubProtocol_wire_v2_encodePayload(void* handle __attribute__(( return pubsubProtocol_encodePayload(message, outBuffer, outLength); } -celix_status_t pubsubProtocol_wire_v2_encodeMetadata(void* handle __attribute__((unused)), pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength) { - return pubsubProtocol_encodeMetadata(message, outBuffer, outLength); +celix_status_t pubsubProtocol_wire_v2_encodeMetadata(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut, size_t* bufferContentLengthOut) { + return pubsubProtocol_encodeMetadata(message, (char**)bufferInOut, bufferLengthInOut, bufferContentLengthOut); } celix_status_t pubsubProtocol_wire_v2_decodePayload(void* handle __attribute__((unused)), void *data, size_t length, pubsub_protocol_message_t *message) { diff --git a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.h b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.h index 295a7a2f7..22df6ec9e 100644 --- a/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.h +++ b/bundles/pubsub/pubsub_protocol/pubsub_protocol_wire_v2/src/pubsub_wire_v2_protocol_impl.h @@ -47,7 +47,7 @@ celix_status_t pubsubProtocol_wire_v2_decodeHeader(void* handle, void *data, siz celix_status_t pubsubProtocol_wire_v2_decodeFooter(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); celix_status_t pubsubProtocol_wire_v2_encodePayload(void* handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); -celix_status_t pubsubProtocol_wire_v2_encodeMetadata(void* handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); +celix_status_t pubsubProtocol_wire_v2_encodeMetadata(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut, size_t* bufferContentLengthOut); celix_status_t pubsubProtocol_wire_v2_decodePayload(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); celix_status_t pubsubProtocol_wire_v2_decodeMetadata(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); diff --git a/bundles/pubsub/pubsub_spi/include/pubsub_protocol.h b/bundles/pubsub/pubsub_spi/include/pubsub_protocol.h index 5fcf20516..7f5d62bdc 100644 --- a/bundles/pubsub/pubsub_spi/include/pubsub_protocol.h +++ b/bundles/pubsub/pubsub_spi/include/pubsub_protocol.h @@ -28,8 +28,8 @@ extern "C" { #include "celix_properties.h" #define PUBSUB_PROTOCOL_SERVICE_NAME "pubsub_protocol" -#define PUBSUB_PROTOCOL_SERVICE_VERSION "1.0.0" -#define PUBSUB_PROTOCOL_SERVICE_RANGE "[1,2)" +#define PUBSUB_PROTOCOL_SERVICE_VERSION "2.0.0" +#define PUBSUB_PROTOCOL_SERVICE_RANGE "[2,3)" typedef struct pubsub_protocol_header pubsub_protocol_header_t; @@ -83,7 +83,8 @@ struct pubsub_protocol_message { typedef struct pubsub_protocol_service { void* handle; /** - * Returns the size of the header. + * @brief Returns the size of the header. + * * Is used by the receiver to configure the expected size of the header. * The receiver first reads the header to know if the receive is big enough * to contain the complete payload. @@ -94,7 +95,8 @@ typedef struct pubsub_protocol_service { */ celix_status_t (*getHeaderSize)(void *handle, size_t *length); /** - * Returns the size of the header buffer for the receiver. + * @brief Returns the size of the header buffer for the receiver. + * * Is used by the receiver to configure the buffer size of the header. * Note for a protocol with a header the headerBufferSize >= headerSize. * Note for header-less protocol the headerBufferSize is zero @@ -106,7 +108,8 @@ typedef struct pubsub_protocol_service { */ celix_status_t (*getHeaderBufferSize)(void *handle, size_t *length); /** - * Returns the size of the sync word + * @brief Returns the size of the sync word + * * Is used by the receiver to skip the sync in the header buffer, * to get in sync with data reception. * @@ -116,7 +119,8 @@ typedef struct pubsub_protocol_service { */ celix_status_t (*getSyncHeaderSize)(void *handle, size_t *length); /** - * Returns the header (as byte array) that should be used by the underlying protocol as sync between messages. + * @brief Returns the header (as byte array) that should be used by the underlying protocol as sync between + * messages. * * @param handle handle for service * @param sync output param for byte array @@ -125,7 +129,8 @@ typedef struct pubsub_protocol_service { celix_status_t (*getSyncHeader)(void *handle, void *sync); /** - * Returns the size of the footer. + * @brief Returns the size of the footer. + * * Is used by the receiver to configure the expected size of the footer. * The receiver reads the footer to know if the complete message including paylaod is received. * @@ -136,7 +141,8 @@ typedef struct pubsub_protocol_service { celix_status_t (*getFooterSize)(void *handle, size_t *length); /** - * Returns the if the protocol service supports the message segmentation attributes that is used by the underlying protocol. + * @brief Returns the if the protocol service supports the message segmentation attributes that is used by the + * underlying protocol. * * @param handle handle for service * @param isSupported indicates that message segmentation is supported or not. @@ -145,19 +151,24 @@ typedef struct pubsub_protocol_service { celix_status_t (*isMessageSegmentationSupported)(void *handle, bool *isSupported); /** - * Encodes the header using the supplied message.header. + * @brief Encode the header using the supplied message.header. + * + * If *bufferInOut is NULL a new buffer will be allocated. + * If *bufferInOut is not NULL and *bufferLengthInOut matches the header size the buffer will be reused. + * If *bufferInOut is not NULL, but *bufferLengthInOut does not match the header size, the provided buffer will + * be freed and a new buffer (matching the header size) will be allocated. * * @param handle handle for service * @param message message to use header from - * @param outBuffer byte array containing the encoded header - * @param outLength length of the byte array + * @param bufferInOut byte array containing the encoded header + * @param bufferLengthInOut length of the byte array * @return status code indicating failure or success */ - celix_status_t (*encodeHeader)(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); + celix_status_t (*encodeHeader)(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut); /** - * Encodes the payload using the supplied message.header. Note, this decode is for protocol specific tasks, and does not perform - * the needed message serialization. See the serialization service for that. + * @brief Encode the payload using the supplied message.header. Note, this encoding is for protocol specific tasks, + * and does not perform the needed message serialization. See the serialization service for that. * In most cases this will simply use the known data and length from message.payload. * * @param handle handle for service @@ -169,30 +180,44 @@ typedef struct pubsub_protocol_service { celix_status_t (*encodePayload)(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); /** - * Encodes the metadata using the supplied message.metadata. + * @brief Encode metadata to bufferInOut using the supplied message.metadata + * + * If *bufferInOut is NULL, a new buffer will be allocated. If *bufferInOut is not NULL, the buffer is reused and + * the provided *bufferLengthInOut must indicate the length of the provided buffer. + * If a provided *bufferInOut is not large enough to fit the encoded metadata, the buffer will be reallocated and + * enlarged. + * + * If this calls return with an error, the caller is still owner of a possible returned output buffer. * - * @param handle handle for service - * @param message message to use header from - * @param outBuffer byte array containing the encoded metadata - * @param outLength length of the byte array - * @return status code indicating failure or success - */ - celix_status_t (*encodeMetadata)(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); + * @param handle handle for service + * @param message The message containing the metadata to encode + * @param bufferInOut Input/output argument for the buffer, if call is successful will contain the metadata header + * @param bufferLengthInOut Input/output arguments for the length of the bufferInOut argument. + * @param bufferContentLengthOut Output argument for the actual content size of the bufferInOut. Note that the + * bufferContentLengthOut can be smaller than the buffer length. + * @return CELIX_SUCCESS if encoding was successful. + */ + celix_status_t (*encodeMetadata)(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut, size_t* bufferContentLengthOut); /** - * Encodes the footer + * @brief Encode the footer + * + * If *bufferInOut is NULL a new buffer will be allocated. + * If *bufferInOut is not NULL and *bufferLengthInOut matches the footer size the buffer will be reused. + * If *bufferInOut is not NULL, but *bufferLengthInOut does not match the footer size, the provided buffer will + * be freed and a new buffer (matching the footer size) will be allocated. * * @param handle handle for service * @param message message to use footer from - * @param outBuffer byte array containing the encoded footer - * @param outLength length of the byte array + * @param bufferInOut byte array containing the encoded footer + * @param bufferLengthInOut length of the byte array * @return status code indicating failure or success */ - celix_status_t (*encodeFooter)(void *handle, pubsub_protocol_message_t *message, void **outBuffer, size_t *outLength); + celix_status_t (*encodeFooter)(void *handle, pubsub_protocol_message_t *message, void **bufferInOut, size_t *bufferLengthInOut); /** - * Decodes the given data into message.header. + * @brief Decodes the given data into message.header. * * @param handle handle for service * @param data incoming byte array to decode @@ -203,8 +228,9 @@ typedef struct pubsub_protocol_service { celix_status_t (*decodeHeader)(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); /** - * Decodes the given data into message.payload. Note, this decode is for protocol specific tasks, and does not perform - * the needed message serialization. See the serialization service for that. + * @brief Decodes the given data into message.payload. Note, this decode is for protocol specific tasks, and + * does not performthe needed message serialization. See the serialization service for that. + * * In most cases this will simply set the incoming data and length in message.payload. * * @param handle handle for service @@ -216,7 +242,7 @@ typedef struct pubsub_protocol_service { celix_status_t (*decodePayload)(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); /** - * Decodes the given data into message.metadata. + * @brief Decodes the given data into message.metadata. * * @param handle handle for service * @param data incoming byte array to decode @@ -227,7 +253,7 @@ typedef struct pubsub_protocol_service { celix_status_t (*decodeMetadata)(void* handle, void *data, size_t length, pubsub_protocol_message_t *message); /** - * Decodes the given data into message.header. + * @brief Decodes the given data into message.header. * * @param handle handle for service * @param data incoming byte array to decode diff --git a/bundles/remote_services/discovery_common/include/endpoint_discovery_server.h b/bundles/remote_services/discovery_common/include/endpoint_discovery_server.h index cf72eabff..7bf05839a 100644 --- a/bundles/remote_services/discovery_common/include/endpoint_discovery_server.h +++ b/bundles/remote_services/discovery_common/include/endpoint_discovery_server.h @@ -81,7 +81,7 @@ celix_status_t endpointDiscoveryServer_removeEndpoint(endpoint_discovery_server_ * @param url [out] url which is used to announce the endpoints. * @return CELIX_SUCCESS when successful. */ -celix_status_t endpointDiscoveryServer_getUrl(endpoint_discovery_server_t *server, char* url); +celix_status_t endpointDiscoveryServer_getUrl(endpoint_discovery_server_t *server, char* url, size_t maxLenUrl); #endif /* ENDPOINT_DISCOVERY_SERVER_H_ */ diff --git a/bundles/remote_services/discovery_common/src/discovery_activator.c b/bundles/remote_services/discovery_common/src/discovery_activator.c index 2c54d835a..3ebfe0ea9 100644 --- a/bundles/remote_services/discovery_common/src/discovery_activator.c +++ b/bundles/remote_services/discovery_common/src/discovery_activator.c @@ -102,20 +102,18 @@ celix_status_t bundleActivator_start(void * userData, celix_bundle_context_t *co return CELIX_ILLEGAL_STATE; } - size_t len = 11 + strlen(OSGI_FRAMEWORK_OBJECTCLASS) + strlen(OSGI_RSA_ENDPOINT_FRAMEWORK_UUID) + strlen(uuid); - char *scope = malloc(len + 1); - if (!scope) { - return CELIX_ENOMEM; - } - - sprintf(scope, "(&(%s=*)(%s=%s))", OSGI_FRAMEWORK_OBJECTCLASS, OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, uuid); - scope[len] = 0; + char* scope = NULL; + int rc = asprintf(&scope, "(&(%s=*)(%s=%s))", OSGI_FRAMEWORK_OBJECTCLASS, OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, uuid); + status = rc < 0 ? CELIX_ENOMEM : CELIX_SUCCESS; - celix_logHelper_debug(activator->loghelper, "using scope %s.", scope); + celix_properties_t *props = NULL; + if (status == CELIX_SUCCESS) { + celix_logHelper_debug(activator->loghelper, "using scope %s.", scope); - celix_properties_t *props = celix_properties_create(); - celix_properties_set(props, "DISCOVERY", "true"); - celix_properties_set(props, (char *) OSGI_ENDPOINT_LISTENER_SCOPE, scope); + props = celix_properties_create(); + celix_properties_set(props, "DISCOVERY", "true"); + celix_properties_set(props, (char *) OSGI_ENDPOINT_LISTENER_SCOPE, scope); + } if (status == CELIX_SUCCESS) { status = serviceTracker_open(activator->endpointListenerTracker); @@ -139,7 +137,10 @@ celix_status_t bundleActivator_start(void * userData, celix_bundle_context_t *co activator->endpointListener = endpointListener; } } - } + } else { + celix_properties_destroy(props); + } + // We can release the scope, as celix_properties_set makes a copy of the key & value... free(scope); diff --git a/bundles/remote_services/discovery_common/src/endpoint_discovery_server.c b/bundles/remote_services/discovery_common/src/endpoint_discovery_server.c index d09ca2af1..0f1eb7e43 100644 --- a/bundles/remote_services/discovery_common/src/endpoint_discovery_server.c +++ b/bundles/remote_services/discovery_common/src/endpoint_discovery_server.c @@ -16,13 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -/** - * endpoint_discovery_server.c - * - * \date Aug 12, 2014 - * \author Apache Celix Project Team - * \copyright Apache License, Version 2.0 - */ #include #include @@ -209,13 +202,13 @@ celix_status_t endpointDiscoveryServer_create(discovery_t *discovery, return status; } -celix_status_t endpointDiscoveryServer_getUrl(endpoint_discovery_server_t *server, char* url) -{ +celix_status_t endpointDiscoveryServer_getUrl(endpoint_discovery_server_t *server, char* url, size_t maxLenUrl) { + celix_status_t status = CELIX_BUNDLE_EXCEPTION; if (server->ip && server->port && server->path) { - sprintf(url, "http://%s:%s/%s", server->ip, server->port, server->path); - status = CELIX_SUCCESS; + int written = snprintf(url, maxLenUrl, "http://%s:%s/%s", server->ip, server->port, server->path); + status = written < maxLenUrl && written > 0 ? CELIX_SUCCESS : CELIX_ILLEGAL_ARGUMENT; } return status; diff --git a/bundles/remote_services/discovery_etcd/src/etcd_watcher.c b/bundles/remote_services/discovery_etcd/src/etcd_watcher.c index c7ada82af..b3d6c70cf 100644 --- a/bundles/remote_services/discovery_etcd/src/etcd_watcher.c +++ b/bundles/remote_services/discovery_etcd/src/etcd_watcher.c @@ -148,7 +148,7 @@ static celix_status_t etcdWatcher_addOwnFramework(etcd_watcher_t *watcher) return status; } - if (endpointDiscoveryServer_getUrl(server, url) != CELIX_SUCCESS) { + if (endpointDiscoveryServer_getUrl(server, url, MAX_VALUE_LENGTH) != CELIX_SUCCESS) { snprintf(url, MAX_VALUE_LENGTH, "http://%s:%s/%s", DEFAULT_SERVER_IP, DEFAULT_SERVER_PORT, DEFAULT_SERVER_PATH); } diff --git a/bundles/remote_services/discovery_shm/src/discovery_shmWatcher.c b/bundles/remote_services/discovery_shm/src/discovery_shmWatcher.c index 8f2493285..d9571dc07 100644 --- a/bundles/remote_services/discovery_shm/src/discovery_shmWatcher.c +++ b/bundles/remote_services/discovery_shm/src/discovery_shmWatcher.c @@ -164,7 +164,7 @@ static void* discoveryShmWatcher_run(void* data) { celix_logHelper_log(discovery->loghelper, CELIX_LOG_LEVEL_WARNING, "Cannot retrieve local discovery path."); } - if (endpointDiscoveryServer_getUrl(discovery->server, &url[0]) != CELIX_SUCCESS) { + if (endpointDiscoveryServer_getUrl(discovery->server, &url[0], MAX_LOCALNODE_LENGTH) != CELIX_SUCCESS) { snprintf(url, MAX_LOCALNODE_LENGTH, "http://%s:%s/%s", DEFAULT_SERVER_IP, DEFAULT_SERVER_PORT, DEFAULT_SERVER_PATH); } diff --git a/bundles/remote_services/topology_manager/tms_tst/disc_mock/disc_mock_activator.c b/bundles/remote_services/topology_manager/tms_tst/disc_mock/disc_mock_activator.c index 40e26f331..708f0eac2 100644 --- a/bundles/remote_services/topology_manager/tms_tst/disc_mock/disc_mock_activator.c +++ b/bundles/remote_services/topology_manager/tms_tst/disc_mock/disc_mock_activator.c @@ -72,18 +72,16 @@ celix_status_t bundleActivator_start(void * userData, celix_bundle_context_t *co return CELIX_ILLEGAL_STATE; } - size_t len = 11 + strlen(OSGI_FRAMEWORK_OBJECTCLASS) + strlen(OSGI_RSA_ENDPOINT_FRAMEWORK_UUID) + strlen(uuid); - char *scope = malloc(len + 1); - if (!scope) { - return CELIX_ENOMEM; - } - - sprintf(scope, "(&(%s=*)(%s=%s))", OSGI_FRAMEWORK_OBJECTCLASS, OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, uuid); - scope[len] = 0; + char* scope = NULL; + int rc = asprintf(&scope, "(&(%s=*)(%s=%s))", OSGI_FRAMEWORK_OBJECTCLASS, OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, uuid); + status = rc < 0 ? CELIX_ENOMEM : CELIX_SUCCESS; - celix_properties_t *props = celix_properties_create(); - celix_properties_set(props, "DISCOVERY", "true"); - celix_properties_set(props, (char *) OSGI_ENDPOINT_LISTENER_SCOPE, scope); + celix_properties_t *props = NULL; + if (status == CELIX_SUCCESS) { + props = celix_properties_create(); + celix_properties_set(props, "DISCOVERY", "true"); + celix_properties_set(props, (char *) OSGI_ENDPOINT_LISTENER_SCOPE, scope); + } if (status == CELIX_SUCCESS) { endpoint_listener_t *endpointListener = calloc(1, sizeof(struct endpoint_listener)); @@ -97,14 +95,19 @@ celix_status_t bundleActivator_start(void * userData, celix_bundle_context_t *co if (status == CELIX_SUCCESS) { act->endpointListener = endpointListener; + props = NULL; } else { free(endpointListener); } } } + // We can release the scope, as celix_properties_set makes a copy of the key & value... free(scope); + //if properties are not used in service registration, destroy it. + celix_properties_destroy(props); + return status; } diff --git a/examples/celix-examples/services_example_cxx/src/DynamicConsumerBundleActivator.cc b/examples/celix-examples/services_example_cxx/src/DynamicConsumerBundleActivator.cc index 5c2bc67ca..a4a38a034 100644 --- a/examples/celix-examples/services_example_cxx/src/DynamicConsumerBundleActivator.cc +++ b/examples/celix-examples/services_example_cxx/src/DynamicConsumerBundleActivator.cc @@ -149,7 +149,7 @@ namespace /*anon*/ { std::lock_guard lock{mutex}; int nextConsumerId = 1; while (consumers.size() < MAX_CONSUMERS) { - ctx->logInfo("Creating dynamic consumer nr %i", consumers.size()); + ctx->logInfo("Creating dynamic consumer nr %zu", consumers.size()); auto consumer = DynamicConsumer::create(nextConsumerId++); consumers[consumer] = ctx->trackServices() .addSetWithPropertiesCallback([consumer](std::shared_ptr calc, std::shared_ptr properties) { diff --git a/libs/dfi/src/avrobin_serializer.c b/libs/dfi/src/avrobin_serializer.c index ef6f6f6f1..3effba471 100644 --- a/libs/dfi/src/avrobin_serializer.c +++ b/libs/dfi/src/avrobin_serializer.c @@ -692,7 +692,8 @@ static int avrobinSerializer_writeSequence(dyn_type *type, void *loc, FILE *stre static int avrobinSerializer_writeEnum(dyn_type *type, void *loc, FILE *stream) { char enum_value_str[16]; - if (sprintf(enum_value_str, "%d", *(int32_t*)loc) < 0) { + int rc = snprintf(enum_value_str, sizeof(enum_value_str), "%d", *(int32_t*)loc); + if (rc >= sizeof(enum_value_str) || rc < 0) { return ERROR; } @@ -1101,7 +1102,8 @@ static int generate_sync(uint8_t **result) { static int generate_record_name(char **result) { char num_str[16]; - if (sprintf(num_str, "R%u", record_name_counter) < 0) { + int rc = snprintf(num_str, sizeof(num_str), "R%u", record_name_counter); + if (rc >= sizeof(num_str) || rc < 0) { return ERROR; } size_t len = strlen(num_str); diff --git a/libs/framework/include/celix/BundleActivator.h b/libs/framework/include/celix/BundleActivator.h index 1656db26c..34cab2502 100644 --- a/libs/framework/include/celix/BundleActivator.h +++ b/libs/framework/include/celix/BundleActivator.h @@ -70,7 +70,7 @@ namespace impl { "\n"; auto ctx = weakCtx.lock(); if (ctx) { - ctx->logWarn(msg.c_str()); + ctx->logWarn("%s", msg.c_str()); } else { std::cout << msg; } diff --git a/libs/framework/include/celix/BundleContext.h b/libs/framework/include/celix/BundleContext.h index 20af2000e..e300e6f59 100644 --- a/libs/framework/include/celix/BundleContext.h +++ b/libs/framework/include/celix/BundleContext.h @@ -598,11 +598,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the TRACE log level. + * @brief Log a message to the Celix framework logger using the TRACE log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logTrace(const char* format...) { + void logTrace(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_TRACE, format, args); @@ -610,11 +610,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the DEBUG log level. + * @brief Log a message to the Celix framework logger using the DEBUG log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logDebug(const char* format...) { + void logDebug(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_DEBUG, format, args); @@ -622,11 +622,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the INFO log level. + * @brief Log a message to the Celix framework logger using the INFO log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logInfo(const char* format...) { + void logInfo(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_INFO, format, args); @@ -634,11 +634,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the WARNING log level. + * @brief Log a message to the Celix framework logger using the WARNING log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logWarn(const char* format...) { + void logWarn(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_WARNING, format, args); @@ -646,11 +646,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the ERROR log level. + * @brief Log a message to the Celix framework logger using the ERROR log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logError(const char* format...) { + void logError(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_ERROR, format, args); @@ -658,11 +658,11 @@ namespace celix { } /** - * @brief Logs a message to the Celix framework logger using the FATAL log level. + * @brief Log a message to the Celix framework logger using the FATAL log level. * * @note Only supports printf style call (so use c_str() instead of std::string) */ - void logFatal(const char* format...) { + void logFatal(const char* format...) __attribute__((format(printf,2,3))) { va_list args; va_start(args, format); celix_bundleContext_vlog(cCtx.get(), CELIX_LOG_LEVEL_FATAL, format, args); diff --git a/libs/framework/include/celix_bundle_context.h b/libs/framework/include/celix_bundle_context.h index 406bbfbd1..473497d26 100644 --- a/libs/framework/include/celix_bundle_context.h +++ b/libs/framework/include/celix_bundle_context.h @@ -1258,12 +1258,20 @@ celix_framework_t* celix_bundleContext_getFramework(const celix_bundle_context_t * @param format printf style format string * @param ... printf style format arguments */ -void celix_bundleContext_log(const celix_bundle_context_t* ctx, celix_log_level_e level, const char* format, ...); +void celix_bundleContext_log( + const celix_bundle_context_t* ctx, + celix_log_level_e level, + const char* format, + ...) __attribute__((format(printf,3,4))); /** * @brief Logs a message to Celix framework logger with the provided log level. */ -void celix_bundleContext_vlog(const celix_bundle_context_t* ctx, celix_log_level_e level, const char* format, va_list formatArgs); +void celix_bundleContext_vlog( + const celix_bundle_context_t* ctx, + celix_log_level_e level, + const char* format, + va_list formatArgs) __attribute__((format(printf,3,0))); /** diff --git a/libs/framework/include/celix_log.h b/libs/framework/include/celix_log.h index f7060947d..661896922 100644 --- a/libs/framework/include/celix_log.h +++ b/libs/framework/include/celix_log.h @@ -52,13 +52,33 @@ void celix_frameworkLogger_destroy(celix_framework_logger_t* logger); void celix_frameworkLogger_setLogCallback(celix_framework_logger_t* logger, void* logHandle, void (*logFunction)(void* handle, celix_log_level_e level, const char* file, const char *function, int line, const char *format, va_list formatArgs)); celix_framework_logger_t* celix_frameworkLogger_globalLogger(); -void celix_framework_log(celix_framework_logger_t* logger, celix_log_level_e level, const char *func, const char *file, int line, - const char *format, ...); +void celix_framework_log( + celix_framework_logger_t* logger, + celix_log_level_e level, + const char *func, + const char *file, + int line, + const char *format, + ...) __attribute__((format(printf,6,7))); -void celix_framework_logCode(celix_framework_logger_t* logger, celix_log_level_e level, const char *func, const char *file, int line, - celix_status_t code, const char *format, ...); +void celix_framework_logCode( + celix_framework_logger_t* logger, + celix_log_level_e level, + const char *func, + const char *file, + int line, + celix_status_t code, + const char *format, + ...) __attribute__((format(printf,7,8))); -void celix_framework_vlog(celix_framework_logger_t* logger, celix_log_level_e level, const char* file, const char* function, int line, const char* format, va_list args); +void celix_framework_vlog( + celix_framework_logger_t* logger, + celix_log_level_e level, + const char* file, + const char* function, + int line, + const char* format, + va_list args) __attribute__((format(printf,6,0))); #ifdef __cplusplus } diff --git a/libs/framework/src/bundle_context.c b/libs/framework/src/bundle_context.c index 073b027a8..6433b1656 100644 --- a/libs/framework/src/bundle_context.c +++ b/libs/framework/src/bundle_context.c @@ -1542,7 +1542,7 @@ long celix_bundleContext_trackServiceTrackersInternal( void *doneData, void (*doneCallback)(void*)) { if (serviceName == NULL) { - fw_log(ctx->framework->logger, CELIX_LOG_LEVEL_DEBUG, "Note starting a meta tracker for all services", __FUNCTION__); + fw_log(ctx->framework->logger, CELIX_LOG_LEVEL_DEBUG, "Note starting a meta tracker for all services"); } celix_bundle_context_service_tracker_tracker_entry_t *entry = calloc(1, sizeof(*entry)); diff --git a/libs/framework/src/celix_log.c b/libs/framework/src/celix_log.c index ccdb38170..7cf34905f 100644 --- a/libs/framework/src/celix_log.c +++ b/libs/framework/src/celix_log.c @@ -118,7 +118,7 @@ static void celix_framework_vlogInternal(celix_framework_logger_t* logger, celix vfprintf(logger->stream, format, args); fputc('\0', logger->stream); //note not sure if this is needed fflush(logger->stream); - celix_logUtils_logToStdoutDetails(LOG_NAME, level, file, function, line, logger->buf); + celix_logUtils_logToStdoutDetails(LOG_NAME, level, file, function, line, "%s", logger->buf); } else { celix_logUtils_vLogToStdoutDetails(LOG_NAME, level, file, function, line, format, args); } diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c index 9bc8cdf4b..b26c8b79a 100644 --- a/libs/framework/src/framework.c +++ b/libs/framework/src/framework.c @@ -76,7 +76,7 @@ static inline void fw_bundleEntry_waitTillUseCountIs(celix_framework_bundle_entr if (entry->useCount != desiredUseCount) { struct timespec now = celix_gettime(CLOCK_MONOTONIC); if (celix_difftime(&start, &now) > 5) { - fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_WARNING, "Bundle '%s' (bnd id = %li) still in use. Use count is %u, desired is %li", celix_bundle_getSymbolicName(entry->bnd), entry->bndId, entry->useCount, desiredUseCount); + fw_log(celix_frameworkLogger_globalLogger(), CELIX_LOG_LEVEL_WARNING, "Bundle '%s' (bnd id = %li) still in use. Use count is %zu, desired is %li", celix_bundle_getSymbolicName(entry->bnd), entry->bndId, entry->useCount, desiredUseCount); start = celix_gettime(CLOCK_MONOTONIC); } } @@ -326,7 +326,7 @@ celix_status_t framework_destroy(framework_pt framework) { bundle_t *bnd = entry->bnd; if (count > 0) { const char *bndName = celix_bundle_getSymbolicName(bnd); - fw_log(framework->logger, CELIX_LOG_LEVEL_FATAL, "Cannot destroy framework. The use count of bundle %s (bnd id %li) is not 0, but %u.", bndName, entry->bndId, count); + fw_log(framework->logger, CELIX_LOG_LEVEL_FATAL, "Cannot destroy framework. The use count of bundle %s (bnd id %li) is not 0, but %zu.", bndName, entry->bndId, count); celixThreadMutex_lock(&framework->dispatcher.mutex); int nrOfRequests = framework->dispatcher.eventQueueSize + celix_arrayList_size(framework->dispatcher.dynamicEventQueue); celixThreadMutex_unlock(&framework->dispatcher.mutex); @@ -1902,7 +1902,7 @@ long celix_framework_registerService(framework_t *fw, celix_bundle_t *bnd, const } else if (serviceName != NULL) { status = celix_serviceRegistry_registerService(fw->registry, bnd, serviceName, svc, properties, 0, ®); } else { - fw_log(fw->logger, CELIX_LOG_LEVEL_ERROR, "Invalid arguments serviceName", serviceName); + fw_log(fw->logger, CELIX_LOG_LEVEL_ERROR, "Invalid arguments serviceName: %s", serviceName); status = CELIX_ILLEGAL_ARGUMENT; } diff --git a/libs/utils/include/celix_log_utils.h b/libs/utils/include/celix_log_utils.h index b90cb4d53..b7763d0fc 100644 --- a/libs/utils/include/celix_log_utils.h +++ b/libs/utils/include/celix_log_utils.h @@ -31,33 +31,40 @@ extern "C" { #endif /** - * Converts the celix log level enum to a const char* value. + * Convert a celix log level enum to a const char* value. */ const char* celix_logUtils_logLevelToString(celix_log_level_e level); /** - * Converts a const char* value to a celix log level + * Convert a const char* value to a celix log level * If the provided log level cannot be parsed or is NULL, the fallbackLogLevel is returned. */ celix_log_level_e celix_logUtils_logLevelFromString(const char *level, celix_log_level_e fallbackLogLevel); /** - * Converts a const char* value to a celix log level + * Convert a const char* value to a celix log level * If the provided log level cannot be parsed or is NULL, the fallbackLogLevel is returned and * if a convertedSuccessfully pointer is provided this will be set to false. - * If converted succcessfully and the convertedSuccessfully pointer is provided. This will be set to true. + * If converted successfully and the convertedSuccessfully pointer is provided. This will be set to true. */ -celix_log_level_e celix_logUtils_logLevelFromStringWithCheck(const char *level, celix_log_level_e fallbackLogLevel, bool *convertedSuccessfully); +celix_log_level_e celix_logUtils_logLevelFromStringWithCheck( + const char *level, + celix_log_level_e fallbackLogLevel, + bool *convertedSuccessfully); /** - * Logs a message to stdout/stderr using the provided logName and log level. + * Log a message to stdout/stderr using the provided logName and log level. * If the provided log level is higher than info, stderr will be used. * */ -void celix_logUtils_logToStdout(const char *logName, celix_log_level_e level, const char *format, ...); +void celix_logUtils_logToStdout( + const char *logName, + celix_log_level_e level, + const char *format, + ...) __attribute__((format(printf,3,4))); /** - * Logs a detailed message to stdout/stderr using the provided logName and log level. + * Log a detailed message to stdout/stderr using the provided logName and log level. * If the provided log level is higher than info, stderr will be used. * * The file, function and line arguments are expected to be called with the values: @@ -66,18 +73,29 @@ void celix_logUtils_logToStdout(const char *logName, celix_log_level_e level, co * If the argument file or function is NULL, the arguments file, function and line are not used. * */ -void celix_logUtils_logToStdoutDetails(const char *logName, celix_log_level_e level, const char* file, const char* function, int line, const char *format, ...); +void celix_logUtils_logToStdoutDetails( + const char *logName, + celix_log_level_e level, + const char* file, + const char* function, + int line, + const char *format, + ...) __attribute__((format(printf,6,7))); /** - * Logs a message to stdout/stderr using the provided logName and log level. + * Log a message to stdout/stderr using the provided logName and log level. * If the provided log level is higher than info, stderr will be used. * */ -void celix_logUtils_vLogToStdout(const char *logName, celix_log_level_e level, const char *format, va_list formatArgs); +void celix_logUtils_vLogToStdout( + const char *logName, + celix_log_level_e level, + const char *format, + va_list formatArgs) __attribute__((format(printf,3,0))); /** - * Logs - a detailed - messages to stdout/stderr using the provided logName and log level. + * Log - a detailed - messages to stdout/stderr using the provided logName and log level. * If the provided log level is higher than info, stderr will be used. * * The file, function and line arguments are expected to be called with the values: @@ -86,11 +104,17 @@ void celix_logUtils_vLogToStdout(const char *logName, celix_log_level_e level, c * If the argument file or function is NULL, the arguments file, function and line are not used. * */ -void celix_logUtils_vLogToStdoutDetails(const char *logName, celix_log_level_e level, const char* file, const char* function, int line, const char *format, va_list formatArgs); +void celix_logUtils_vLogToStdoutDetails( + const char *logName, + celix_log_level_e level, + const char* file, + const char* function, + int line, + const char *format, + va_list formatArgs) __attribute__((format(printf,6,0))); /** - * Prints a backtrace to the provided output stream. - * + * Print a backtrace to the provided output stream. */ void celix_logUtils_printBacktrace(FILE* stream); diff --git a/libs/utils/private/test/array_list_test.cpp b/libs/utils/private/test/array_list_test.cpp index cb5f652cd..da90de5a1 100644 --- a/libs/utils/private/test/array_list_test.cpp +++ b/libs/utils/private/test/array_list_test.cpp @@ -127,7 +127,7 @@ TEST(array_list, clone) { for (i = 0; i < 12; i++) { bool added; char entry[11]; - sprintf(entry, "|%s|%d|", "entry", i); + snprintf(entry, sizeof(entry), "|%s|%d|", "entry", i); added = arrayList_add(list, my_strdup(entry)); CHECK(added); } @@ -144,7 +144,7 @@ TEST(array_list, clone) { for (j = 0; j < 12; j++) { void *entry = NULL; char entrys[11]; - sprintf(entrys, "|%s|%d|", "entry", j); + snprintf(entrys, sizeof(entrys), "|%s|%d|", "entry", j); entry = arrayList_get(clone, j); STRCMP_EQUAL((char *) entry, entrys); diff --git a/libs/utils/private/test/hash_map_test.cpp b/libs/utils/private/test/hash_map_test.cpp index 003c02191..e3fb34da5 100644 --- a/libs/utils/private/test/hash_map_test.cpp +++ b/libs/utils/private/test/hash_map_test.cpp @@ -390,15 +390,14 @@ TEST(hash_map, resize){ LONGS_EQUAL(16, map->tablelength); LONGS_EQUAL(12, map->treshold); for (i = 0; i < 12; i++) { - char key[6]; - sprintf(key, "key%d", i); + snprintf(key, sizeof(key), "key%d", i); hashMap_put(map, my_strdup(key), my_strdup(key)); } LONGS_EQUAL(12, map->size); LONGS_EQUAL(16, map->tablelength); LONGS_EQUAL(12, map->treshold); - sprintf(key, "key%d", i); + snprintf(key, sizeof(key), "key%d", i); hashMap_put(map, my_strdup(key), my_strdup(key)); LONGS_EQUAL(13, map->size); LONGS_EQUAL(32, map->tablelength); diff --git a/libs/utils/src/version_range.c b/libs/utils/src/version_range.c index 10e61e190..c190ccfec 100644 --- a/libs/utils/src/version_range.c +++ b/libs/utils/src/version_range.c @@ -263,47 +263,25 @@ char* celix_versionRange_createLDAPFilter(const celix_version_range_t* range, co } bool celix_versionRange_createLDAPFilterInPlace(const celix_version_range_t* range, const char *serviceVersionAttributeName, char* buffer, size_t bufferLength) { - if(buffer == NULL || bufferLength == 0) { - return false; - } - - const char* format = "(&(%s%s%i.%i.%i)(%s%s%i.%i.%i))"; - if(range->high == NULL) { - format = "(&(%s%s%i.%i.%i))"; - } - - // check if buffer is long enough - int size = 0; - if(range->high == NULL) { - size = snprintf(NULL, 0, format, - serviceVersionAttributeName, range->isLowInclusive ? ">=" : ">", range->low->major, - range->low->minor, range->low->micro); - } else { - size = snprintf(NULL, 0, format, - serviceVersionAttributeName, range->isLowInclusive ? ">=" : ">", range->low->major, - range->low->minor, range->low->micro, - serviceVersionAttributeName, range->isHighInclusive ? "<=" : "<", range->high->major, - range->high->minor, range->high->micro); - } - - if(size >= bufferLength || size < 0) { + if(buffer == NULL || bufferLength == 0 || range->low == NULL) { return false; } + int size; // write contents into buffer - if(range->high == NULL) { - size = snprintf(buffer, bufferLength, format, + if (range->high == NULL) { + size = snprintf(buffer, bufferLength, "(&(%s%s%i.%i.%i))", serviceVersionAttributeName, range->isLowInclusive ? ">=" : ">", range->low->major, range->low->minor, range->low->micro); } else { - size = snprintf(buffer, bufferLength, format, + size = snprintf(buffer, bufferLength, "(&(%s%s%i.%i.%i)(%s%s%i.%i.%i))", serviceVersionAttributeName, range->isLowInclusive ? ">=" : ">", range->low->major, range->low->minor, range->low->micro, serviceVersionAttributeName, range->isHighInclusive ? "<=" : "<", range->high->major, range->high->minor, range->high->micro); } - if(size >= bufferLength || size < 0) { + if (size >= bufferLength || size < 0) { return false; }