From bfe6ffb450eccb749f5b5cf2dfda913fdc6e55c3 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 27 Sep 2026 23:02:39 +0200 Subject: [PATCH 1/4] Improve remote shell loop error handling --- .../remote_shell/src/connection_listener.c | 8 +++- bundles/shell/remote_shell/src/remote_shell.c | 40 +++++++++++-------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/bundles/shell/remote_shell/src/connection_listener.c b/bundles/shell/remote_shell/src/connection_listener.c index a1af23e9a..5b53816c1 100644 --- a/bundles/shell/remote_shell/src/connection_listener.c +++ b/bundles/shell/remote_shell/src/connection_listener.c @@ -83,7 +83,7 @@ celix_status_t connectionListener_create(remote_shell_pt remoteShell, int port, celix_status_t connectionListener_start(connection_listener_pt instance) { celix_status_t status = CELIX_SUCCESS; celixThreadMutex_lock(&instance->mutex); - celixThread_create(&instance->thread, NULL, connection_listener_thread, instance); + status = celixThread_create(&instance->thread, NULL, connection_listener_thread, instance); celixThreadMutex_unlock(&instance->mutex); return status; } @@ -138,7 +138,11 @@ static void* connection_listener_thread(void *data) { char portStr[10]; snprintf(&portStr[0], 10, "%d", instance->port); - getaddrinfo(NULL, portStr, &hints, &result); + int gaiResult = getaddrinfo(NULL, portStr, &hints, &result); + if (gaiResult != 0) { + celix_logHelper_log(*instance->loghelper, CELIX_LOG_LEVEL_ERROR, "getaddrinfo failed: %s", gai_strerror(gaiResult)); + return NULL; + } for (rp = result; rp != NULL && status == CELIX_BUNDLE_EXCEPTION; rp = rp->ai_next) { diff --git a/bundles/shell/remote_shell/src/remote_shell.c b/bundles/shell/remote_shell/src/remote_shell.c index e16ef2148..d7261774b 100644 --- a/bundles/shell/remote_shell/src/remote_shell.c +++ b/bundles/shell/remote_shell/src/remote_shell.c @@ -175,24 +175,32 @@ void *remoteShell_connection_run(void *data) { char buff[COMMAND_BUFF_SIZE]; len = recv(fd, buff, COMMAND_BUFF_SIZE - 1, 0); - if (len < COMMAND_BUFF_SIZE) { - celix_status_t commandStatus = CELIX_SUCCESS; - buff[len] = '\0'; - - commandStatus = remoteShell_connection_execute(connection, buff); - - if (commandStatus == CELIX_SUCCESS) { - remoteShell_connection_print(connection, RS_PROMPT); - } else if (commandStatus == CELIX_FILE_IO_EXCEPTION) { - //exit command - break; - } else { //error - remoteShell_connection_print(connection, RS_ERROR); - remoteShell_connection_print(connection, RS_PROMPT); + if (len > 0) { + if (len < COMMAND_BUFF_SIZE) { + celix_status_t commandStatus = CELIX_SUCCESS; + buff[len] = '\0'; + + commandStatus = remoteShell_connection_execute(connection, buff); + + if (commandStatus == CELIX_SUCCESS) { + remoteShell_connection_print(connection, RS_PROMPT); + } else if (commandStatus == CELIX_FILE_IO_EXCEPTION) { + //exit command + break; + } else { //error + remoteShell_connection_print(connection, RS_ERROR); + remoteShell_connection_print(connection, RS_PROMPT); + } + + } else { + celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: Error while retrieving data"); } - + } else if (len == 0) { + celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_INFO, "REMOTE_SHELL: Connection closed by peer"); + break; } else { - celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: Error while retrieving data"); + celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: recv() failed: %s", strerror(errno)); + break; } } } From 0b909e60120aaf994a1ad89fee5ea1d4548b3d8b Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sat, 10 Oct 2026 19:57:21 +0200 Subject: [PATCH 2/4] Improve read loops for http admin, rsa and remote shell --- CHANGES.md | 1 + .../http_admin/http_admin/src/http_admin.c | 123 +++++++----------- .../src/remote_service_admin_dfi.c | 71 +++++----- bundles/shell/remote_shell/src/remote_shell.c | 30 +++-- 4 files changed, 112 insertions(+), 113 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 3eec41940..a9883c4ec 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -123,6 +123,7 @@ limitations under the License. - Improved `INSTALL_RPATH` to use `$ORIGIN` during installation. - Corrected bundle update behavior when updating bundles from different sources. - Enhanced `libcurl` initialization procedures and made `libcurl` optional. +- HTTP Admin, Remote Service Admin (DFI) and Remote Shell now keep reading until the full request body / command is received. ## Fixes diff --git a/bundles/http_admin/http_admin/src/http_admin.c b/bundles/http_admin/http_admin/src/http_admin.c index 7db8e1ea5..769120199 100755 --- a/bundles/http_admin/http_admin/src/http_admin.c +++ b/bundles/http_admin/http_admin/src/http_admin.c @@ -56,6 +56,7 @@ typedef struct http_alias { //Local function prototypes static int http_request_handle(struct mg_connection *connection); +static bool httpAdmin_readRequestBody(struct mg_connection *connection, long long content_length, char **bufferOut, size_t *bytesReadOut); static void httpAdmin_updateInfoSvc(http_admin_manager_t *admin); static void createAliasesSymlink(const char *aliases, const char *admin_root, const char *bundle_root, long bundle_id, celix_array_list_t *alias_list); static bool aliasList_containsAlias(celix_array_list_t *alias_list, const char *alias); @@ -173,6 +174,34 @@ void http_admin_removeHttpService(void *handle, void *svc CELIX_UNUSED, const ce } } +static bool httpAdmin_readRequestBody(struct mg_connection* connection, long long contentLength, char** bufferOut, size_t* bytesReadOut) { + char* buffer = NULL; + size_t totalBytesRead = 0; + bool success = true; + if (contentLength > 0) { + size_t length = (contentLength > INT_MAX) ? INT_MAX : (size_t)contentLength; + buffer = malloc(length + 1); + if (buffer == NULL) { + success = false; + } else { + while (totalBytesRead < length) { + int read = mg_read(connection, buffer + totalBytesRead, length - totalBytesRead); + if (read <= 0) { + success = false; + break; + } + totalBytesRead += (size_t)read; + } + if (success) { + buffer[totalBytesRead] = '\0'; + } + } + } + *bufferOut = buffer; + *bytesReadOut = totalBytesRead; + return success; +} + int http_request_handle(struct mg_connection *connection) { int ret_status = 400; //Default bad request @@ -208,59 +237,32 @@ int http_request_handle(struct mg_connection *connection) { } } else if (strcmp("POST", ri->request_method) == 0) { if (httpSvc->doPost != NULL) { - int bytes_read = 0; - bool no_data = false; char *rcv_buf = NULL; - - if (ri->content_length > 0) { - int content_size = (ri->content_length > INT_MAX ? INT_MAX : (int) ri->content_length); - rcv_buf = malloc((size_t) content_size + 1); - rcv_buf[content_size] = '\0'; - bytes_read = mg_read(connection, rcv_buf, (size_t) content_size); - } else { - no_data = true; - } - - if (bytes_read > 0 || no_data) { - ret_status = httpSvc->doPost(httpSvc->handle, connection, rcv_buf, (size_t) bytes_read); + size_t bytesRead = 0; + bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); + if (success) { + ret_status = httpSvc->doPost(httpSvc->handle, connection, rcv_buf, bytesRead); } else { mg_send_http_error(connection, 400, "%s", "Bad request"); ret_status = 400; //Bad Request, failed to read data } - - if (rcv_buf != NULL) { - free(rcv_buf); - } + free(rcv_buf); } else { ret_status = 0; //Let civetweb handle the request } } else if (strcmp("PUT", ri->request_method) == 0) { if (httpSvc->doPut != NULL) { - int bytes_read = 0; - bool no_data = false; char *rcv_buf = NULL; - - if (ri->content_length > 0) { - int content_size = (ri->content_length > INT_MAX ? INT_MAX : (int) ri->content_length); - rcv_buf = malloc((size_t) content_size + 1); - rcv_buf[content_size] = '\0'; - bytes_read = mg_read(connection, rcv_buf, (size_t) content_size); - } else { - no_data = true; - } - - if (bytes_read > 0 || no_data) { - ret_status = httpSvc->doPut(httpSvc->handle, connection, req_uri, rcv_buf, - (size_t) bytes_read); + size_t bytesRead = 0; + bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); + if (success) { + ret_status = httpSvc->doPut(httpSvc->handle, connection, req_uri, rcv_buf, bytesRead); } else { mg_send_http_error(connection, 400, "%s", "Bad request"); ret_status = 400; //Bad Request, failed to read data } - - if (rcv_buf != NULL) { - free(rcv_buf); - } + free(rcv_buf); } else { ret_status = 0; //Let civetweb handle the request } @@ -272,29 +274,16 @@ int http_request_handle(struct mg_connection *connection) { } } else if (strcmp("TRACE", ri->request_method) == 0) { if (httpSvc->doTrace != NULL) { - int bytes_read = 0; - bool no_data = false; char *rcv_buf = NULL; - - if (ri->content_length > 0) { - int content_size = (ri->content_length > INT_MAX ? INT_MAX : (int) ri->content_length); - rcv_buf = malloc((size_t) content_size + 1); - rcv_buf[content_size] = '\0'; - bytes_read = mg_read(connection, rcv_buf, (size_t) content_size); - } else { - no_data = true; - } - - if (bytes_read > 0 || no_data) { - ret_status = httpSvc->doTrace(httpSvc->handle, connection, rcv_buf, (size_t) bytes_read); + size_t bytesRead = 0; + bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); + if (success) { + ret_status = httpSvc->doTrace(httpSvc->handle, connection, rcv_buf, bytesRead); } else { mg_send_http_error(connection, 400, "%s", "Bad request"); ret_status = 400; //Bad Request, failed to read data } - - if (rcv_buf != NULL) { - free(rcv_buf); - } + free(rcv_buf); } else { ret_status = 0; //Let civetweb handle the request } @@ -306,30 +295,16 @@ int http_request_handle(struct mg_connection *connection) { } } else if (strcmp("PATCH", ri->request_method) == 0) { if (httpSvc->doPatch != NULL) { - int bytes_read = 0; - bool no_data = false; char *rcv_buf = NULL; - - if (ri->content_length > 0) { - int content_size = (ri->content_length > INT_MAX ? INT_MAX : (int) ri->content_length); - rcv_buf = malloc((size_t) content_size + 1); - rcv_buf[content_size] = '\0'; - bytes_read = mg_read(connection, rcv_buf, (size_t) content_size); - } else { - no_data = true; - } - - if (bytes_read > 0 || no_data) { - ret_status = httpSvc->doPatch(httpSvc->handle, connection, req_uri, rcv_buf, - (size_t) bytes_read); + size_t bytesRead = 0; + bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); + if (success) { + ret_status = httpSvc->doPatch(httpSvc->handle, connection, req_uri, rcv_buf, bytesRead); } else { mg_send_http_error(connection, 400, "%s", "Bad request"); ret_status = 400; //Bad Request, failed to read data } - - if (rcv_buf != NULL) { - free(rcv_buf); - } + free(rcv_buf); } else { ret_status = 0; //Let civetweb handle the request } diff --git a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c index 5c02b5bbe..6f3521595 100644 --- a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c +++ b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c @@ -528,41 +528,54 @@ static int remoteServiceAdmin_callback(struct mg_connection *conn) { if (export != NULL) { uint64_t datalength = request_info->content_length; char* data = malloc(datalength + 1); - mg_read(conn, data, datalength); - data[datalength] = '\0'; - - char *response = NULL; - int responceLength = 0; - int rc = exportRegistration_call(export, data, -1, &metadata, &response, &responceLength); - if (rc != CELIX_SUCCESS) { - RSA_LOG_ERROR(rsa, "Error trying to invoke remove service, got error %i\n", rc); + size_t readLen = 0; + while (readLen < datalength) { + int this_read = mg_read(conn, data + readLen, (size_t)(datalength - readLen)); + if (this_read <= 0) { + break; + } + readLen += (size_t)this_read; } + data[readLen] = '\0'; - if (rc == CELIX_SUCCESS && response != NULL) { - mg_write(conn, data_response_headers, strlen(data_response_headers)); - - char *bufLoc = response; - size_t bytesLeft = strlen(response); - if (bytesLeft > INT_MAX) { - //NOTE arcording to civetweb mg_write, there is a limit on mg_write for INT_MAX. - RSA_LOG_WARNING(rsa, "nr of bytes to send for a remote call is > INT_MAX, this can lead to issues\n"); + if (readLen < datalength) { + RSA_LOG_ERROR(rsa, "Error while reading request body, received %zu of %llu bytes", readLen, (unsigned long long)datalength); + mg_send_http_error(conn, 400, "%s", "Bad request"); + result = 400; + } else { + char *response = NULL; + int responceLength = 0; + int rc = exportRegistration_call(export, data, -1, &metadata, &response, &responceLength); + if (rc != CELIX_SUCCESS) { + RSA_LOG_ERROR(rsa, "Error trying to invoke remove service, got error %i\n", rc); } - while (bytesLeft > 0) { - int send = mg_write(conn, bufLoc, strlen(bufLoc)); - if (send > 0) { - bytesLeft -= send; - bufLoc += send; - } else { - RSA_LOG_ERROR(rsa, "Error sending response: %s", strerror(errno)); - break; + + if (rc == CELIX_SUCCESS && response != NULL) { + mg_write(conn, data_response_headers, strlen(data_response_headers)); + + char *bufLoc = response; + size_t bytesLeft = strlen(response); + if (bytesLeft > INT_MAX) { + //NOTE arcording to civetweb mg_write, there is a limit on mg_write for INT_MAX. + RSA_LOG_WARNING(rsa, "nr of bytes to send for a remote call is > INT_MAX, this can lead to issues\n"); + } + while (bytesLeft > 0) { + int send = mg_write(conn, bufLoc, strlen(bufLoc)); + if (send > 0) { + bytesLeft -= send; + bufLoc += send; + } else { + RSA_LOG_ERROR(rsa, "Error sending response: %s", strerror(errno)); + break; + } } - } - free(response); - } else { - mg_write(conn, no_content_response_headers, strlen(no_content_response_headers)); + free(response); + } else { + mg_write(conn, no_content_response_headers, strlen(no_content_response_headers)); + } + result = 1; } - result = 1; free(data); exportRegistration_decreaseUsage(export); diff --git a/bundles/shell/remote_shell/src/remote_shell.c b/bundles/shell/remote_shell/src/remote_shell.c index d7261774b..0b3aba4a0 100644 --- a/bundles/shell/remote_shell/src/remote_shell.c +++ b/bundles/shell/remote_shell/src/remote_shell.c @@ -151,10 +151,13 @@ celix_status_t remoteShell_stopConnections(remote_shell_pt instance) { void *remoteShell_connection_run(void *data) { celix_status_t status = CELIX_SUCCESS; connection_pt connection = data; - size_t len; + ssize_t len; int result; struct timeval timeout; /* Timeout for select */ + char buff[COMMAND_BUFF_SIZE]; + size_t used = 0; + int fd = fileno(connection->socketStream); connection->threadRunning = true; @@ -172,28 +175,35 @@ void *remoteShell_connection_run(void *data) { /* The socket_fd has data available to be read */ if (result > 0 && FD_ISSET(fd, &connection->pollset)) { - char buff[COMMAND_BUFF_SIZE]; - - len = recv(fd, buff, COMMAND_BUFF_SIZE - 1, 0); + len = recv(fd, buff + used, COMMAND_BUFF_SIZE - 1 - used, 0); if (len > 0) { - if (len < COMMAND_BUFF_SIZE) { - celix_status_t commandStatus = CELIX_SUCCESS; - buff[len] = '\0'; + used += (size_t)len; + char *newline = NULL; + while (status == CELIX_SUCCESS && (newline = memchr(buff, '\n', used)) != NULL) { + size_t consumed = (size_t)(newline - buff) + 1; + *newline = '\0'; - commandStatus = remoteShell_connection_execute(connection, buff); + celix_status_t commandStatus = remoteShell_connection_execute(connection, buff); if (commandStatus == CELIX_SUCCESS) { remoteShell_connection_print(connection, RS_PROMPT); } else if (commandStatus == CELIX_FILE_IO_EXCEPTION) { //exit command + status = CELIX_FILE_IO_EXCEPTION; break; } else { //error remoteShell_connection_print(connection, RS_ERROR); remoteShell_connection_print(connection, RS_PROMPT); } - } else { - celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: Error while retrieving data"); + memmove(buff, buff + consumed, used - consumed); + used -= consumed; + } + + if (status == CELIX_SUCCESS && used >= COMMAND_BUFF_SIZE - 1) { + //Buffer is full without a newline, so no complete command could be read. + celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: Received data without a newline, dropping data"); + used = 0; } } else if (len == 0) { celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_INFO, "REMOTE_SHELL: Connection closed by peer"); From dd06cb44821f4df8ce1dd38965e8cc0b322cdc4c Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 18 Oct 2026 18:49:36 +0200 Subject: [PATCH 3/4] Improve err handling rsa and add max request body size for rsa dfi --- CHANGES.md | 1 + .../http_admin/http_admin/src/http_admin.c | 86 +++++--- .../remote_service_admin_dfi/README.md | 3 + .../src/remote_service_admin_dfi.c | 207 ++++++++++-------- .../src/remote_service_admin_dfi_constants.h | 13 ++ 5 files changed, 190 insertions(+), 120 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index a9883c4ec..2c4edba4a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -124,6 +124,7 @@ limitations under the License. - Corrected bundle update behavior when updating bundles from different sources. - Enhanced `libcurl` initialization procedures and made `libcurl` optional. - HTTP Admin, Remote Service Admin (DFI) and Remote Shell now keep reading until the full request body / command is received. +- RSA DFI now limits the accepted request body size (configurable via `CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB`, default 10 (MB)). ## Fixes diff --git a/bundles/http_admin/http_admin/src/http_admin.c b/bundles/http_admin/http_admin/src/http_admin.c index 769120199..7e860c0c4 100755 --- a/bundles/http_admin/http_admin/src/http_admin.c +++ b/bundles/http_admin/http_admin/src/http_admin.c @@ -31,6 +31,11 @@ #include "celix_compiler.h" #include "celix_threads.h" +#include "celix_stdlib_cleanup.h" + +#define CELIX_HTTP_ADMIN_READ_ERROR_MSG "Bad Request: Read error" +#define CELIX_HTTP_ADMIN_INCOMPLETE_BODY_MSG "Bad Request: Incomplete Body" +#define CELIX_HTTP_ADMIN_INSUFFICIENT_MEM_MSG "Server Error: Insufficient memory" struct http_admin_manager { @@ -56,7 +61,11 @@ typedef struct http_alias { //Local function prototypes static int http_request_handle(struct mg_connection *connection); -static bool httpAdmin_readRequestBody(struct mg_connection *connection, long long content_length, char **bufferOut, size_t *bytesReadOut); +/** + * @brief read the request body. + * @return 0 if succesfull, > 0 and a error code (4xx or 5xx) if not sucessfull. + */ +static int httpAdmin_readRequestBody(struct mg_connection *connection, long long content_length, char **bufferOut, size_t *bytesReadOut, const char** errorMsgOut); static void httpAdmin_updateInfoSvc(http_admin_manager_t *admin); static void createAliasesSymlink(const char *aliases, const char *admin_root, const char *bundle_root, long bundle_id, celix_array_list_t *alias_list); static bool aliasList_containsAlias(celix_array_list_t *alias_list, const char *alias); @@ -174,32 +183,37 @@ void http_admin_removeHttpService(void *handle, void *svc CELIX_UNUSED, const ce } } -static bool httpAdmin_readRequestBody(struct mg_connection* connection, long long contentLength, char** bufferOut, size_t* bytesReadOut) { - char* buffer = NULL; +static int httpAdmin_readRequestBody(struct mg_connection* connection, long long contentLength, char** bufferOut, size_t* bytesReadOut, const char** errorMsgOut) { + celix_autofree char* buffer = NULL; size_t totalBytesRead = 0; - bool success = true; + *errorMsgOut = NULL; if (contentLength > 0) { size_t length = (contentLength > INT_MAX) ? INT_MAX : (size_t)contentLength; buffer = malloc(length + 1); if (buffer == NULL) { - success = false; - } else { - while (totalBytesRead < length) { - int read = mg_read(connection, buffer + totalBytesRead, length - totalBytesRead); - if (read <= 0) { - success = false; - break; - } - totalBytesRead += (size_t)read; - } - if (success) { - buffer[totalBytesRead] = '\0'; + *errorMsgOut = CELIX_HTTP_ADMIN_INSUFFICIENT_MEM_MSG; + return 500; + } + + while (totalBytesRead < length) { + int read = mg_read(connection, buffer + totalBytesRead, length - totalBytesRead); + if (read == 0) { + break; //EOF + } else if (read < 0) { + *errorMsgOut = CELIX_HTTP_ADMIN_READ_ERROR_MSG; + return 400; } + totalBytesRead += (size_t)read; + } + if (totalBytesRead < length) { + *errorMsgOut = CELIX_HTTP_ADMIN_INCOMPLETE_BODY_MSG; + return 400; } + buffer[totalBytesRead] = '\0'; } - *bufferOut = buffer; + *bufferOut = celix_steal_ptr(buffer); *bytesReadOut = totalBytesRead; - return success; + return 0; } int http_request_handle(struct mg_connection *connection) { @@ -239,12 +253,13 @@ int http_request_handle(struct mg_connection *connection) { if (httpSvc->doPost != NULL) { char *rcv_buf = NULL; size_t bytesRead = 0; - bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); - if (success) { + const char* errMsg; + int errCode = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead, &errMsg); + if (errCode == 0) { ret_status = httpSvc->doPost(httpSvc->handle, connection, rcv_buf, bytesRead); } else { - mg_send_http_error(connection, 400, "%s", "Bad request"); - ret_status = 400; //Bad Request, failed to read data + mg_send_http_error(connection, errCode, "%s", errMsg); + ret_status = errCode; //set status to the error code, so the request is not passed to civetweb } free(rcv_buf); } else { @@ -255,12 +270,13 @@ int http_request_handle(struct mg_connection *connection) { if (httpSvc->doPut != NULL) { char *rcv_buf = NULL; size_t bytesRead = 0; - bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); - if (success) { + const char* errMsg; + int errCode = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead, &errMsg); + if (errCode == 0) { ret_status = httpSvc->doPut(httpSvc->handle, connection, req_uri, rcv_buf, bytesRead); } else { - mg_send_http_error(connection, 400, "%s", "Bad request"); - ret_status = 400; //Bad Request, failed to read data + mg_send_http_error(connection, errCode, "%s", errMsg); + ret_status = errCode; //set status to the error code, so the request is not passed to civetweb } free(rcv_buf); } else { @@ -276,12 +292,13 @@ int http_request_handle(struct mg_connection *connection) { if (httpSvc->doTrace != NULL) { char *rcv_buf = NULL; size_t bytesRead = 0; - bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); - if (success) { + const char* errMsg = NULL; + int errCode = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead, &errMsg); + if (errCode == 0) { ret_status = httpSvc->doTrace(httpSvc->handle, connection, rcv_buf, bytesRead); } else { - mg_send_http_error(connection, 400, "%s", "Bad request"); - ret_status = 400; //Bad Request, failed to read data + mg_send_http_error(connection, errCode, "%s", errMsg); + ret_status = errCode; //set status to the error code, so the request is not passed to civetweb } free(rcv_buf); } else { @@ -297,12 +314,13 @@ int http_request_handle(struct mg_connection *connection) { if (httpSvc->doPatch != NULL) { char *rcv_buf = NULL; size_t bytesRead = 0; - bool success = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead); - if (success) { + const char* errMsg; + int errCode = httpAdmin_readRequestBody(connection, ri->content_length, &rcv_buf, &bytesRead, &errMsg); + if (errCode == 0) { ret_status = httpSvc->doPatch(httpSvc->handle, connection, req_uri, rcv_buf, bytesRead); } else { - mg_send_http_error(connection, 400, "%s", "Bad request"); - ret_status = 400; //Bad Request, failed to read data + mg_send_http_error(connection, errCode, "%s", errMsg); + ret_status = errCode; //set status to the error code, so the request is not passed to civetweb } free(rcv_buf); } else { diff --git a/bundles/remote_services/remote_service_admin_dfi/README.md b/bundles/remote_services/remote_service_admin_dfi/README.md index 2dfb6523e..903b27dee 100644 --- a/bundles/remote_services/remote_service_admin_dfi/README.md +++ b/bundles/remote_services/remote_service_admin_dfi/README.md @@ -41,5 +41,8 @@ Libffi is configured using descriptor files in the bundles. CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT If set to true the RSA will support dynamic IP address fill-in for service exports. Default is false. + CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB The maximum size of the accepted request body in megabytes. Requests with a body larger + than this limit are rejected with an HTTP 413 response. Default is 10 MB. + ###### CMake option RSA_REMOTE_SERVICE_ADMIN_DFI=ON diff --git a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c index 6f3521595..b3b7151e8 100644 --- a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c +++ b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -103,6 +104,8 @@ struct remote_service_admin { char *discoveryInterface; bool dynamicIpSupport; + size_t maxRequestBodySize; + struct mg_context *ctx; FILE *logFile; @@ -136,7 +139,7 @@ static const char *data_response_headers = "\r\n"; static const char *no_content_response_headers = - "HTTP/1.1 204 OK\r\n"; + "HTTP/1.1 204 OK\r\n\r\n"; static const unsigned int DEFAULT_TIMEOUT = 0; @@ -217,6 +220,12 @@ celix_status_t remoteServiceAdmin_create(celix_bundle_context_t *context, remote (*admin)->dynamicIpSupport = celix_bundleContext_getPropertyAsBool(context, CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT, CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT_DEFAULT); + long maxRequestBodySizeInMb = celix_bundleContext_getPropertyAsLong(context, CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB, CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB_DEFAULT); + if (maxRequestBodySizeInMb <= 0) { + maxRequestBodySizeInMb = CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB_DEFAULT; + } + (*admin)->maxRequestBodySize = (size_t)maxRequestBodySizeInMb * 1024ul * 1024ul; + char *detectedIp = NULL; if ((interface != NULL) && (remoteServiceAdmin_getIpAddress((char*)interface, &detectedIp) != CELIX_SUCCESS)) { celix_logHelper_log((*admin)->loghelper, CELIX_LOG_LEVEL_WARNING, "RSA: Could not retrieve IP address for interface %s", interface); @@ -458,40 +467,42 @@ celix_status_t remoteServiceAdmin_stop(remote_service_admin_t *admin) { celix_status_t importRegistration_getFactory(import_registration_t *import, service_factory_pt *factory); static int remoteServiceAdmin_callback(struct mg_connection *conn) { - int result = 1; // zero means: let civetweb handle it further, any non-zero value means it is handled by us... export_registration_t *export = NULL; - celix_properties_t *metadata = NULL; + celix_autoptr(celix_properties_t) metadata = NULL; const struct mg_request_info *request_info = mg_get_request_info(conn); - if (request_info->request_uri != NULL) { - remote_service_admin_t *rsa = request_info->user_data; - - - if (strncmp(request_info->request_uri, "/service/", 9) == 0 && strcmp("POST", request_info->request_method) == 0) { - - // uri = /services/myservice/call - const char *uri = request_info->request_uri; - // rest = myservice/call - - const char *rest = uri+9; - char *interfaceStart = strchr(rest, '/'); - int pos = interfaceStart - rest; - char service[pos+1]; - strncpy(service, rest, pos); - service[pos] = '\0'; - unsigned long serviceId = strtoul(service,NULL,10); - - for (int i = 0; i < request_info->num_headers; i++) { - struct mg_header header = request_info->http_headers[i]; - if (strncmp(header.name, "X-RSA-Metadata-", 15) == 0) { - if (metadata == NULL) { - metadata = celix_properties_create(); - } - celix_properties_set(metadata, header.name + 15, header.value); + if (request_info->request_uri == NULL) { + return 0; + } + + remote_service_admin_t *rsa = request_info->user_data; + if (strncmp(request_info->request_uri, "/service/", 9) == 0 && strcmp("POST", request_info->request_method) == 0) { + + // uri = /services/myservice/call + const char *uri = request_info->request_uri; + // rest = myservice/call + + const char *rest = uri+9; + const char *interfaceStart = strchr(rest, '/'); + int pos = interfaceStart - rest; + char service[pos+1]; + strncpy(service, rest, pos); + service[pos] = '\0'; + unsigned long serviceId = strtoul(service,NULL,10); + + for (int i = 0; i < request_info->num_headers; i++) { + struct mg_header header = request_info->http_headers[i]; + if (strncmp(header.name, "X-RSA-Metadata-", 15) == 0) { + if (metadata == NULL) { + metadata = celix_properties_create(); } + celix_properties_set(metadata, header.name + 15, header.value); } + } + - celixThreadRwlock_readLock(&rsa->exportedServicesLock); + { + celix_auto(celix_rwlock_rlock_guard_t) rLockGuard = celixRwlockRlockGuard_init(&rsa->exportedServicesLock); //find endpoint hash_map_iterator_pt iter = hashMapIterator_create(rsa->exportedServices); @@ -518,76 +529,100 @@ static int remoteServiceAdmin_callback(struct mg_connection *conn) { if (export != NULL) { exportRegistration_increaseUsage(export); } else { - result = 0; RSA_LOG_WARNING(rsa, "No export registration found for service id %lu", serviceId); + return 0; } - celixThreadRwlock_unlock(&rsa->exportedServicesLock); } + } else { + //no rsa call, let civetweb handle. + return 0; + } + assert(export != NULL); + long long contentLength = request_info->content_length; //-1 if no content length was provided + size_t maxRequestBodySize = rsa->maxRequestBodySize; + size_t readLen = 0; - if (export != NULL) { - uint64_t datalength = request_info->content_length; - char* data = malloc(datalength + 1); - size_t readLen = 0; - while (readLen < datalength) { - int this_read = mg_read(conn, data + readLen, (size_t)(datalength - readLen)); - if (this_read <= 0) { - break; - } - readLen += (size_t)this_read; - } - data[readLen] = '\0'; - - if (readLen < datalength) { - RSA_LOG_ERROR(rsa, "Error while reading request body, received %zu of %llu bytes", readLen, (unsigned long long)datalength); - mg_send_http_error(conn, 400, "%s", "Bad request"); - result = 400; - } else { - char *response = NULL; - int responceLength = 0; - int rc = exportRegistration_call(export, data, -1, &metadata, &response, &responceLength); - if (rc != CELIX_SUCCESS) { - RSA_LOG_ERROR(rsa, "Error trying to invoke remove service, got error %i\n", rc); - } + if (contentLength > 0 && (unsigned long long)contentLength > maxRequestBodySize) { + RSA_LOG_ERROR(rsa, "Error while reading request body, the body is larger than the maximum of %zu bytes", maxRequestBodySize); + mg_send_http_error(conn, 413, "%s", "Request body too large"); + exportRegistration_decreaseUsage(export); + return 1; + } + + size_t length = (contentLength > 0) ? (size_t)contentLength : (maxRequestBodySize+1) /*note +1 byte, to check if too body size goes over the limit*/; + celix_autofree char* data = malloc(length + 1); + if (data == NULL) { + RSA_LOG_ERROR(rsa, "Error while allocating a buffer of %zu bytes for the request body", (contentLength > 0) ? (size_t)contentLength : maxRequestBodySize); + mg_send_http_error(conn, 500, "%s", "Internal server error"); + exportRegistration_decreaseUsage(export); + return 1; + } - if (rc == CELIX_SUCCESS && response != NULL) { - mg_write(conn, data_response_headers, strlen(data_response_headers)); + while (readLen < length) { + int this_read = mg_read(conn, data + readLen, length - readLen); + if (this_read == 0) { + break; //EOF + } else if (this_read < 0) { + RSA_LOG_ERROR(rsa, "Error while reading request body, received %zu bytes", readLen); + mg_send_http_error(conn, 400, "%s", "Bad request"); + exportRegistration_decreaseUsage(export); + return 1; + } + readLen += (size_t)this_read; + } - char *bufLoc = response; - size_t bytesLeft = strlen(response); - if (bytesLeft > INT_MAX) { - //NOTE arcording to civetweb mg_write, there is a limit on mg_write for INT_MAX. - RSA_LOG_WARNING(rsa, "nr of bytes to send for a remote call is > INT_MAX, this can lead to issues\n"); - } - while (bytesLeft > 0) { - int send = mg_write(conn, bufLoc, strlen(bufLoc)); - if (send > 0) { - bytesLeft -= send; - bufLoc += send; - } else { - RSA_LOG_ERROR(rsa, "Error sending response: %s", strerror(errno)); - break; - } - } + if (contentLength <= 0 && readLen > maxRequestBodySize) { + RSA_LOG_ERROR(rsa, "Error while reading request body, the body is larger than the maximum of %zu bytes", maxRequestBodySize); + mg_send_http_error(conn, 413, "%s", "Request body too large"); + exportRegistration_decreaseUsage(export); + return 1; + } - free(response); - } else { - mg_write(conn, no_content_response_headers, strlen(no_content_response_headers)); - } - result = 1; + if (contentLength > 0 && readLen < (size_t)contentLength) { + RSA_LOG_ERROR(rsa, "Error while reading request body, received %zu of %llu bytes", readLen, (unsigned long long)contentLength); + mg_send_http_error(conn, 400, "%s", "Bad request"); + exportRegistration_decreaseUsage(export); + return 1; + } + data[readLen] = '\0'; + + char *response = NULL; + int responceLength = 0; + int rc = exportRegistration_call(export, data, -1, &metadata, &response, &responceLength); + if (rc == CELIX_ILLEGAL_ARGUMENT) { + RSA_LOG_ERROR(rsa, "Error trying to invoke remove service, got error %i\n", rc); + mg_send_http_error(conn, 400, "%s", "Bad request"); + } else if (rc != CELIX_SUCCESS) { + RSA_LOG_ERROR(rsa, "Error trying to invoke remove service, got error %i\n", rc); + mg_send_http_error(conn, 500, "%s", "Internal server error"); + } else if (response != NULL) { + mg_write(conn, data_response_headers, strlen(data_response_headers)); + + char *bufLoc = response; + size_t bytesLeft = strlen(response); + if (bytesLeft > INT_MAX) { + //NOTE arcording to civetweb mg_write, there is a limit on mg_write for INT_MAX. + RSA_LOG_WARNING(rsa, "nr of bytes to send for a remote call is > INT_MAX, this can lead to issues\n"); + } + while (bytesLeft > 0) { + int send = mg_write(conn, bufLoc, strlen(bufLoc)); + if (send > 0) { + bytesLeft -= send; + bufLoc += send; + } else { + RSA_LOG_ERROR(rsa, "Error sending response: %s", strerror(errno)); + break; } - - free(data); - exportRegistration_decreaseUsage(export); } - } - //free metadata - if(metadata != NULL) { - celix_properties_destroy(metadata); + free(response); + } else { + mg_write(conn, no_content_response_headers, strlen(no_content_response_headers)); } - return result; + exportRegistration_decreaseUsage(export); + return 1; //DFI has handled the request. } celix_status_t remoteServiceAdmin_exportService(remote_service_admin_t *admin, char *serviceId, celix_properties_t *properties, celix_array_list_t** registrationsOut) { diff --git a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi_constants.h b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi_constants.h index 1adcfaa57..4a5bb7ae5 100644 --- a/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi_constants.h +++ b/bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi_constants.h @@ -79,5 +79,18 @@ */ #define CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT_DEFAULT false +/** + * @brief Remote Service Admin DFI environment property (named "CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB") which + * specifies the maximum size of the request body the RSA server will accept, in megabytes. + * + * The property is of the type long. + */ +#define CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB "CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB" + +/** + * @brief Default value for the property CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB (in MB) + */ +#define CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB_DEFAULT 10L + #endif //CELIX_REMOTE_SERVICE_ADMIN_DFI_CONSTANTS_H From 885c8e432579e9a4725a4ef3af1723843af83655 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 18 Oct 2026 19:36:16 +0200 Subject: [PATCH 4/4] Improve remote shell recv handling --- bundles/shell/remote_shell/src/remote_shell.c | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/bundles/shell/remote_shell/src/remote_shell.c b/bundles/shell/remote_shell/src/remote_shell.c index 0b3aba4a0..a707964f3 100644 --- a/bundles/shell/remote_shell/src/remote_shell.c +++ b/bundles/shell/remote_shell/src/remote_shell.c @@ -49,8 +49,25 @@ struct connection { typedef struct connection *connection_pt; -static celix_status_t remoteShell_connection_print(connection_pt connection, char * text); -static celix_status_t remoteShell_connection_execute(connection_pt connection, char *command); +static inline bool connection_isThreadRunning(connection_pt connection) { + return __atomic_load_n(&connection->threadRunning, __ATOMIC_RELAXED); +} + +static inline void connection_setThreadRunning(connection_pt connection, bool value) { + __atomic_store_n(&connection->threadRunning, value, __ATOMIC_RELAXED); +} + +/** + * @brief Check whether errno indicates a transient receive condition (currently no data available). + */ +static bool connection_isTransientRecvError(void) { + //Note EAGAIN and EWOULDBLOCK may be the same value on some platforms (e.g. Linux) and differ on + //others (e.g. some BSDs). This function can be made platform-dependent. + return errno == EAGAIN || errno == EWOULDBLOCK; +} + +static celix_status_t remoteShell_connection_print(connection_pt connection, const char * text); +static celix_status_t remoteShell_connection_execute(connection_pt connection, const char *command); static void* remoteShell_connection_run(void *data); celix_status_t remoteShell_create(shell_mediator_pt mediator, int maximumConnections, remote_shell_pt* instance) { @@ -96,7 +113,7 @@ celix_status_t remoteShell_addConnection(remote_shell_pt instance, int socket) { if (connection != NULL) { connection->parent = instance; - connection->threadRunning = false; + connection_setThreadRunning(connection, false); connection->socketStream = fdopen(socket, "w"); if (connection->socketStream != NULL) { @@ -140,7 +157,7 @@ celix_status_t remoteShell_stopConnections(remote_shell_pt instance) { for (i = 0; i < length; i += 1) { connection_pt connection = celix_arrayList_get(instance->connections, i); - connection->threadRunning = false; + connection_setThreadRunning(connection, false); } celixThreadMutex_unlock(&instance->mutex); @@ -160,10 +177,10 @@ void *remoteShell_connection_run(void *data) { int fd = fileno(connection->socketStream); - connection->threadRunning = true; + connection_setThreadRunning(connection, true); status = remoteShell_connection_print(connection, RS_WELCOME); - while (status == CELIX_SUCCESS && connection->threadRunning == true) { + while (status == CELIX_SUCCESS && connection_isThreadRunning(connection)) { do { timeout.tv_sec = CONNECTION_LISTENER_TIMEOUT_SEC; timeout.tv_usec = 0; @@ -171,11 +188,14 @@ void *remoteShell_connection_run(void *data) { FD_ZERO(&connection->pollset); FD_SET(fd, &connection->pollset); result = select(fd + 1, &connection->pollset, NULL, NULL, &timeout); - } while (result == -1 && errno == EINTR && connection->threadRunning == true); + } while (result == -1 && errno == EINTR && connection_isThreadRunning(connection)); /* The socket_fd has data available to be read */ if (result > 0 && FD_ISSET(fd, &connection->pollset)) { - len = recv(fd, buff + used, COMMAND_BUFF_SIZE - 1 - used, 0); + do { + len = recv(fd, buff + used, COMMAND_BUFF_SIZE - 1 - used, 0); + } while (len < 0 && errno == EINTR && connection_isThreadRunning(connection)); + if (len > 0) { used += (size_t)len; char *newline = NULL; @@ -199,15 +219,11 @@ void *remoteShell_connection_run(void *data) { memmove(buff, buff + consumed, used - consumed); used -= consumed; } - - if (status == CELIX_SUCCESS && used >= COMMAND_BUFF_SIZE - 1) { - //Buffer is full without a newline, so no complete command could be read. - celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: Received data without a newline, dropping data"); - used = 0; - } } else if (len == 0) { celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_INFO, "REMOTE_SHELL: Connection closed by peer"); break; + } else if (connection_isTransientRecvError()) { + //No data currently available, loop back to select() instead of disconnecting. } else { celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_ERROR, "REMOTE_SHELL: recv() failed: %s", strerror(errno)); break; @@ -227,7 +243,7 @@ void *remoteShell_connection_run(void *data) { return NULL; } -static celix_status_t remoteShell_connection_execute(connection_pt connection, char *command) { +static celix_status_t remoteShell_connection_execute(connection_pt connection, const char *command) { celix_status_t status = CELIX_SUCCESS; if (status == CELIX_SUCCESS) { @@ -249,7 +265,7 @@ static celix_status_t remoteShell_connection_execute(connection_pt connection, c return status; } -celix_status_t remoteShell_connection_print(connection_pt connection, char *text) { +celix_status_t remoteShell_connection_print(connection_pt connection, const char *text) { size_t len = strlen(text); int fd = fileno(connection->socketStream); return (send(fd, text, len, 0) > 0) ? CELIX_SUCCESS : CELIX_FILE_IO_EXCEPTION;