Skip to content

Add std::nothrow and null checks to netsocket lib #14223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions connectivity/netsocket/include/netsocket/NetworkInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ class NetworkInterface: public DNS {
*/
virtual void attach(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);

#if MBED_CONF_NSAPI_ADD_EVENT_LISTENER_RETURN_CHANGE
/** Add event listener for interface.
*
* This API allows multiple callback to be registered for a single interface.
Expand All @@ -387,8 +388,30 @@ class NetworkInterface: public DNS {
* of both leads to undefined behavior.
*
* @param status_cb The callback for status changes.
* @return NSAPI_ERROR_OK on success
* @return NSAPI_ERROR_NO_MEMORY if the function fails to create a new entry.
*/
nsapi_error_t add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);
#else
/** Add event listener for interface.
*
* This API allows multiple callback to be registered for a single interface.
* When first called, internal list of event handlers are created and registered to
* interface through attach() API.
*
* Application may only use attach() or add_event_listener() interface. Mixing usage
* of both leads to undefined behavior.
*
* @warning This version of the function does not use the `std::nothrow` feature. Subsequently,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the spellchecker:

add nothrow to mbed-os/tools/test/travis-ci/doxy-spellchecker/ignore.en.pws

* the function may fail to allocate memory and cause a system error. To use the new
* version with the changes, set "nsapi.add-event-listener-return-change": 1 in the
* target overrides section in your mbed_app.conf file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be mbed_app.json (not conf)

*
* @param status_cb The callback for status changes.
*/
MBED_DEPRECATED_SINCE("mbed-os-6.8", "This function return value will change to nsapi_error_t in the next major release. See documentation for details.")
void add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);
#endif

#if MBED_CONF_PLATFORM_CALLBACK_COMPARABLE
/** Remove event listener from interface.
Expand Down
4 changes: 4 additions & 0 deletions connectivity/netsocket/mbed_lib.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
"name": "nsapi",
"config": {
"present": 1,
"add-event-listener-return-change": {
"help": "Updates the add_event_listener to return a nsapi_error_t value which can indicate allocation failure. See documents for more details.",
"value": 0
},
"default-stack": {
"help" : "Default stack to be used, valid values: LWIP, NANOSTACK.",
"value" : "LWIP"
Expand Down
17 changes: 16 additions & 1 deletion connectivity/netsocket/source/NetworkInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "platform/Callback.h"
#include "platform/mbed_error.h"
#include <string.h>
#include <new>

// Default network-interface state
void NetworkInterface::set_as_default()
Expand Down Expand Up @@ -141,15 +142,29 @@ static void call_all_event_listeners(NetworkInterface *iface, nsapi_event_t even
}
}
}

#if MBED_CONF_NSAPI_ADD_EVENT_LISTENER_RETURN_CHANGE
nsapi_error_t NetworkInterface::add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb)
#else
void NetworkInterface::add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb)
#endif
{
iface_eventlist_t *event_list = get_interface_event_list_head();

#if MBED_CONF_NSAPI_ADD_EVENT_LISTENER_RETURN_CHANGE
iface_eventlist_entry_t *entry = new (std::nothrow) iface_eventlist_entry_t;
if (!entry) {
return NSAPI_ERROR_NO_MEMORY;
}
#else
iface_eventlist_entry_t *entry = new iface_eventlist_entry_t;
#endif
entry->iface = this;
entry->status_cb = status_cb;
ns_list_add_to_end(event_list, entry);
attach(mbed::callback(&call_all_event_listeners, this));
#if MBED_CONF_NSAPI_ADD_EVENT_LISTENER_RETURN_CHANGE
return NSAPI_ERROR_OK;
#endif
}

#if MBED_CONF_PLATFORM_CALLBACK_COMPARABLE
Expand Down
8 changes: 7 additions & 1 deletion connectivity/netsocket/source/TCPSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include "netsocket/TCPSocket.h"
#include <new>
#include "Timer.h"
#include "mbed_assert.h"

Expand Down Expand Up @@ -266,7 +267,12 @@ TCPSocket *TCPSocket::accept(nsapi_error_t *error)
ret = _stack->socket_accept(_socket, &socket, &address);

if (0 == ret) {
connection = new TCPSocket(this, socket, address);
connection = new (std::nothrow) TCPSocket(this, socket, address);
if (!connection) {
_stack->socket_close(socket);
ret = NSAPI_ERROR_NO_MEMORY;
break;
}
_socket_stats.stats_update_peer(connection, address);
_socket_stats.stats_update_socket_state(connection, SOCK_CONNECTED);
break;
Expand Down
26 changes: 20 additions & 6 deletions connectivity/netsocket/source/TLSSocketWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

#include "netsocket/TLSSocketWrapper.h"
#include <new>
#include "platform/Callback.h"
#include "drivers/Timer.h"
#include "events/mbed_events.h"
Expand Down Expand Up @@ -134,7 +135,10 @@ nsapi_error_t TLSSocketWrapper::set_client_cert_key(const void *client_cert, siz
#else

int ret;
mbedtls_x509_crt *crt = new mbedtls_x509_crt;
mbedtls_x509_crt *crt = new (std::nothrow) mbedtls_x509_crt;
if (!crt) {
return NSAPI_ERROR_NO_MEMORY;
}
mbedtls_x509_crt_init(crt);
if ((ret = mbedtls_x509_crt_parse(crt, static_cast<const unsigned char *>(client_cert),
client_cert_len)) != 0) {
Expand Down Expand Up @@ -286,7 +290,11 @@ nsapi_error_t TLSSocketWrapper::continue_handshake()
#if defined(MBEDTLS_X509_CRT_PARSE_C) && defined(FEA_TRACE_SUPPORT) && !defined(MBEDTLS_X509_REMOVE_INFO)
/* Prints the server certificate and verify it. */
const size_t buf_size = 1024;
char *buf = new char[buf_size];
char *buf = new (std::nothrow) char[buf_size];
if (!buf) {
print_mbedtls_error("new (std::nothrow) char[buf_size] failed in continue_handshake", NSAPI_ERROR_NO_MEMORY);
return NSAPI_ERROR_NO_MEMORY;
}
mbedtls_x509_crt_info(buf, buf_size, "\r ",
mbedtls_ssl_get_peer_cert(&_ssl));
tr_debug("Server certificate:\r\n%s\r\n", buf);
Expand Down Expand Up @@ -427,10 +435,9 @@ void TLSSocketWrapper::print_mbedtls_error(MBED_UNUSED const char *name, MBED_UN
{
// Avoid pulling in mbedtls_strerror when trace is not enabled
#if defined FEA_TRACE_SUPPORT && defined MBEDTLS_ERROR_C
char *buf = new char[128];
char buf[128];
mbedtls_strerror(err, buf, 128);
tr_err("%s() failed: -0x%04x (%d): %s", name, -err, err, buf);
delete[] buf;
#else
tr_err("%s() failed: -0x%04x (%d)", name, -err, err);
#endif
Expand Down Expand Up @@ -459,7 +466,11 @@ void TLSSocketWrapper::my_debug(void *ctx, int level, const char *file, int line
int TLSSocketWrapper::my_verify(void *data, mbedtls_x509_crt *crt, int depth, uint32_t *flags)
{
const uint32_t buf_size = 1024;
char *buf = new char[buf_size];
char *buf = new (std::nothrow) char[buf_size];
if (!buf) {
return NSAPI_ERROR_NO_MEMORY;
}

(void) data;

tr_debug("\nVerifying certificate at depth %d:\n", depth);
Expand Down Expand Up @@ -569,7 +580,10 @@ mbedtls_ssl_config *TLSSocketWrapper::get_ssl_config()
{
if (!_ssl_conf) {
int ret;
_ssl_conf = new mbedtls_ssl_config;
_ssl_conf = new (std::nothrow) mbedtls_ssl_config;
if (!_ssl_conf) {
return nullptr;
}
mbedtls_ssl_config_init(_ssl_conf);
_ssl_conf_allocated = true;

Expand Down
7 changes: 6 additions & 1 deletion connectivity/netsocket/source/nsapi_dns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,12 @@ static void nsapi_dns_query_async_create(void *ptr)
}

if (!query->socket_cb_data) {
query->socket_cb_data = new SOCKET_CB_DATA;
query->socket_cb_data = new (std::nothrow) SOCKET_CB_DATA;
if (!query->socket_cb_data) {
delete socket;
nsapi_dns_query_async_resp(query, NSAPI_ERROR_NO_MEMORY, NULL);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return an error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I won't pretend I understand this portion too much, the function signature here is static void nsapi_dns_query_async_create(void *ptr). This function is a helper for the async DNS query, and I believe the nsapi_dns_query_async_resp call takes care of the error return call.

I did not believe I looked hard enough, however, because I believe that I did not clean up the UDP socket and the appropriate call to the stack created earlier in the same function call.

}
}
query->socket_cb_data->call_in_cb = query->call_in_cb;
query->socket_cb_data->stack = query->stack;
Expand Down