Skip to content

Commit

Permalink
Fixes apache#677 - [coverity] double_free: Calling `qd_free_http_adap…
Browse files Browse the repository at this point in the history
…tor_config` frees pointer `config` which has already been freed (apache#683)
  • Loading branch information
jiridanek committed Sep 1, 2022
1 parent 6fa764e commit 74b79c2
Show file tree
Hide file tree
Showing 5 changed files with 187 additions and 21 deletions.
4 changes: 2 additions & 2 deletions .clang-format
Expand Up @@ -39,8 +39,8 @@ IncludeCategories:
# python-related headers must go first
- Regex: '^<Python.h>|"qpid/dispatch/python_embedded.h"|"python_private.h"$'
Priority: -2
# helpers.hpp must go before qdr_doctest.hpp
- Regex: 'helpers.hpp[">]$'
# helpers.hpp must go after qdr_doctest.hpp
- Regex: 'qdr_doctest.hpp[">]$'
Priority: -1
- Regex: '^"qpid/dispatch/?.*\.h"'
Priority: 2
Expand Down
32 changes: 16 additions & 16 deletions src/adaptors/http_common.c
Expand Up @@ -76,18 +76,20 @@ void qd_free_http_adaptor_config(qd_http_adaptor_config_t *config)
free_qd_http_adaptor_config_t(config);
}


qd_error_t qd_load_http_adaptor_config(qd_dispatch_t *qd, qd_http_adaptor_config_t *config, qd_entity_t* entity, qd_log_source_t *log_source)
qd_http_adaptor_config_t *qd_load_http_adaptor_config(qd_dispatch_t *qd, qd_entity_t *entity,
qd_log_source_t *log_source)
{
qd_http_adaptor_config_t *config = qd_http_adaptor_config();

//
// First load the common config data (common to all adaptors)
//
qd_error_t qd_error = qd_load_adaptor_config(qd, config->adaptor_config, entity, log_source);
if (qd_error != QD_ERROR_NONE) {
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR,
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR, //
"Unable to load config information: %s", qd_error_message());
qd_free_http_adaptor_config(config);
return qd_error;
return 0;
}

//
Expand Down Expand Up @@ -120,25 +122,23 @@ qd_error_t qd_load_http_adaptor_config(qd_dispatch_t *qd, qd_http_adaptor_config
aggregation_str = 0;
}

return QD_ERROR_NONE;
return config;

error:
qd_free_http_adaptor_config(config);
free(version_str);
free(aggregation_str);
return qd_error_code();

return 0;
}


qd_http_listener_t *qd_dispatch_configure_http_listener(qd_dispatch_t *qd, qd_entity_t *entity)
{
qd_http_listener_t *listener = 0;
qd_http_adaptor_config_t *config = qd_http_adaptor_config();
if (qd_load_http_adaptor_config(qd, config, entity, qd_log_source(QD_HTTP_LOG_SOURCE)) != QD_ERROR_NONE) {
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR,
qd_http_listener_t *listener = 0;
qd_http_adaptor_config_t *config = qd_load_http_adaptor_config(qd, entity, qd_log_source(QD_HTTP_LOG_SOURCE));
if (!config) {
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR, //
"Unable to create http listener: %s", qd_error_message());
qd_free_http_adaptor_config(config);
return 0;
}

Expand Down Expand Up @@ -194,11 +194,11 @@ qd_error_t qd_entity_refresh_httpListener(qd_entity_t* entity, void *impl)

qd_http_connector_t *qd_dispatch_configure_http_connector(qd_dispatch_t *qd, qd_entity_t *entity)
{
qd_http_connector_t *conn = 0;
qd_http_adaptor_config_t *config = qd_http_adaptor_config();
qd_http_connector_t *conn = 0;
qd_http_adaptor_config_t *config = qd_load_http_adaptor_config(qd, entity, qd_log_source(QD_HTTP_LOG_SOURCE));

if (qd_load_http_adaptor_config(qd, config, entity, qd_log_source(QD_HTTP_LOG_SOURCE)) != QD_ERROR_NONE) {
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR,
if (!config) {
qd_log(qd_log_source(QD_HTTP_LOG_SOURCE), QD_LOG_ERROR, //
"Unable to create http connector: %s", qd_error_message());
return 0;
}
Expand Down
6 changes: 4 additions & 2 deletions src/adaptors/http_common.h
Expand Up @@ -113,8 +113,10 @@ void qd_http_record_request(qdr_core_t *core, const char * method, uint32_t stat
uint64_t bytes_in, uint64_t bytes_out, uint64_t latency);
char *qd_get_host_from_host_port(const char *host_port);

qd_error_t qd_load_http_adaptor_config(qd_dispatch_t *qd, qd_http_adaptor_config_t *config, qd_entity_t* entity, qd_log_source_t *log_source);
void qd_http_free_adaptor_config(qd_http_adaptor_config_t *config);
qd_http_adaptor_config_t *qd_load_http_adaptor_config(qd_dispatch_t *qd, qd_entity_t *entity,
qd_log_source_t *log_source);
void qd_free_http_adaptor_config(qd_http_adaptor_config_t *config);

//
// These functions are defined in their respective HTTP adaptors:
//
Expand Down
6 changes: 5 additions & 1 deletion tests/c_unittests/CMakeLists.txt
Expand Up @@ -19,7 +19,8 @@

# -fno-inline: just to be extra sure, no particular reason so far
# -fno-builtin: GCC would optimize e.g. abs() and we would not be able to stub
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_STANDARD_FLAGS} -fno-inline -fno-builtin -fno-stack-protector")
# -Wno-literal-suffix allows compiling string literals such as "[C%"PRIu64"]" as C++
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_STANDARD_FLAGS} -fno-inline -fno-builtin -fno-stack-protector -Wno-literal-suffix")

add_library(static_accessors_server_c SHARED
static_accessors_server_c.c)
Expand All @@ -34,11 +35,14 @@ add_executable(c_unittests
helpers.hpp
test_amqp.cpp
test_connection_manager_static.cpp
test_http_listener_connector.cpp
test_listener_startup.cpp
test_router_startup.cpp
test_server.cpp
test_terminus.cpp)
target_link_libraries(c_unittests cpp-stub pthread skupper-router static_accessors_server_c)
# http_common.h includes "delivery.h"
target_include_directories(c_unittests PRIVATE ${CMAKE_SOURCE_DIR}/src/router_core)

file(COPY
${CMAKE_CURRENT_SOURCE_DIR}/minimal_silent.conf
Expand Down
160 changes: 160 additions & 0 deletions tests/c_unittests/test_http_listener_connector.cpp
@@ -0,0 +1,160 @@
/*
* 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 <Python.h>

#include "./qdr_doctest.hpp"

#include "./helpers.hpp" // must come after ./qdr_doctest.hpp

#include <map>
#include <thread>
#include <variant>

extern "C" {
#include "adaptors/http_common.h"
}

using entity_value_t = std::variant<std::string, const char *, bool>;

/// Builds a std::unique_ptr<qd_entity_t> instance from a C++ map with std::string-typed keys.
/// Caller must hold the Python GIL (qd_entity_t is a PyObject).
static auto create_entity_from_config_map(std::map<std::string, entity_value_t> const &config)
{
PyObject *pyObject = PyDict_New();
for (auto [k, v] : config) {
PyObject *item = nullptr;
if (std::holds_alternative<std::string>(v)) {
item = PyUnicode_FromString(std::get<std::string>(v).c_str());
} else if (std::holds_alternative<const char *>(v)) {
// gcc 8.5. 0 (on RHEL 8) prefers to cast const char * to bool and not std::string
item = PyUnicode_FromString(std::get<const char *>(v));
} else if (std::holds_alternative<bool>(v)) {
item = std::get<bool>(v) ? Py_True : Py_False;
Py_IncRef(item);
}
REQUIRE_MESSAGE(item != nullptr, "Test utils cannot handle value of a given type");

PyDict_SetItemString(pyObject, k.c_str(), item);
Py_DECREF(item);
}

// return a std::unique_ptr with a deleter the runs Py_DECREF when the value goes out of scope
qd_entity_t *entity = reinterpret_cast<qd_entity_t *>(pyObject);
return qd_make_unique(entity, [](qd_entity_t *entity) { Py_DECREF(entity); });
}

TEST_CASE("Configure and start HTTP_ADAPTOR listener")
{
std::thread([] {
QDR qdr{};
qdr.initialize("./minimal_trace.conf");
qdr.wait();
qd_dispatch_t *qd = qdr.qd;

CaptureCStream css(stderr);

// previous functions dropped Python GIL
auto state = PyGILState_Ensure();

SUBCASE("empty config should fail but not crash")
{
std::map<std::string, entity_value_t> config = {
// empty map
};

// build entity
auto entity = create_entity_from_config_map(config);

// perform test
CHECK(qd_dispatch_configure_http_listener(qd, entity.get()) == nullptr);

// check logging
std::string logging = css.str();
const std::string unable = R"EOS(HTTP_ADAPTOR (error) Unable to load config information)EOS";
CHECK_MESSAGE(logging.find(unable) != std::string::npos, unable, " not found in ", logging);
}

SUBCASE("minimal config should create listener")
{
std::map<std::string, entity_value_t> config = {
{"protocolVersion", "HTTP1"},

{"name", "my_config"}, //
{"host", "localhost"}, //
{"port", "0"}, //
{"address", "my_address"}, //
};

// build entity
auto entity = create_entity_from_config_map(config);

// perform test
qd_http_listener_t *listener = qd_dispatch_configure_http_listener(qd, entity.get());
REQUIRE(listener != nullptr);

// clean up
qd_dispatch_delete_http_listener(qd, listener);

// check logging
std::string logging = css.str();
const std::string configured =
R"EOS(HTTP_ADAPTOR (info) Configured HTTP_ADAPTOR listener on localhost:0)EOS";
CHECK_MESSAGE(logging.find(configured) != std::string::npos, configured, " not found in ", logging);
}
PyGILState_Release(state);

qdr.deinitialize();
}).join();
}

TEST_CASE("Configure and start HTTP connector")
{
QDR qdr{};
qdr.initialize("./minimal_trace.conf");
qdr.wait();
qd_dispatch_t *qd = qdr.qd;

CaptureCStream css(stderr);

// previous functions dropped Python GIL
auto state = PyGILState_Ensure();

SUBCASE("empty config should fail but not crash")
{
std::map<std::string, entity_value_t> config = {
// empty map
};

// build entity
auto entity = create_entity_from_config_map(config);

// perform test
CHECK(qd_dispatch_configure_http_connector(qd, entity.get()) == nullptr);

// check logging
std::string logging = css.str();
const std::string unable = R"EOS(HTTP_ADAPTOR (error) Unable to load config information)EOS";
CHECK_MESSAGE(logging.find(unable) != std::string::npos, unable, " not found in ", logging);
}

PyGILState_Release(state);

qdr.deinitialize();
}

0 comments on commit 74b79c2

Please sign in to comment.