Skip to content

Improve remote shell loop error handling #851

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.

Sign up for GitHub

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

Open
pnoltes wants to merge 4 commits into
base: master
Choose a base branch
from
+282 −185
Open

Improve remote shell loop error handling #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfe6ffb
Improve remote shell loop error handling
pnoltes Sep 27, 2026
0b909e6
Improve read loops for http admin, rsa and remote shell
pnoltes Oct 10, 2026
dd06cb4
Improve err handling rsa and add max request body size for rsa dfi
pnoltes Oct 18, 2026
885c8e4
Improve remote shell recv handling
pnoltes Oct 18, 2026
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
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ 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.
- RSA DFI now limits the accepted request body size (configurable via `CELIX_RSA_DFI_MAX_REQUEST_BODY_SIZE_IN_MB`, default 10 (MB)).

## Fixes

Expand Down
157 changes: 75 additions & 82 deletions bundles/http_admin/http_admin/src/http_admin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

@PengZheng PengZheng Oct 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The status texts like "Bad Request"/"server Error" are redundent according to mg_send_http_error documentation:

The status text (e.g., "Not Found") for standard error codes is known by this function. A body of the error message, to explain the error in more detail, can be specified using the fmt format specifier and additional arguments. The fmt format specifier works like for the printf() function in the standard C library.

Check https://github.com/civetweb/civetweb/blob/588860e30721bf5453b0440c390865a8e85dcae5/examples/rest/rest.c#L94 for example usage.



struct http_admin_manager {
Expand All @@ -56,6 +61,11 @@ typedef struct http_alias {

//Local function prototypes
static int http_request_handle(struct mg_connection *connection);
/**
* @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);
Expand Down Expand Up @@ -173,6 +183,39 @@ void http_admin_removeHttpService(void *handle, void *svc CELIX_UNUSED, const ce
}
}

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;
*errorMsgOut = NULL;
if (contentLength > 0) {
size_t length = (contentLength > INT_MAX) ? INT_MAX : (size_t)contentLength;
buffer = malloc(length + 1);

@PengZheng PengZheng Oct 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shall we apply some upper bound to request body size?
For a request without explicit content-length (Transfer-Encoding: chunked), it is not safe to allocate so much memory.
And it would be nice to have test cases for both such unusually large requests and reqeusts with chunked transfer-encoding.

Or we may explicitly refuse requests without content-length by returning 411 (Length Required).
This way we cannot process chunked requests.

if (buffer == NULL) {
*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) {

@PengZheng PengZheng Oct 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if contentLength == -1? Then we can never receive a request without explicit content_length successfully.

*errorMsgOut = CELIX_HTTP_ADMIN_INCOMPLETE_BODY_MSG;
return 400;
}
buffer[totalBytesRead] = '\0';
}
*bufferOut = celix_steal_ptr(buffer);
*bytesReadOut = totalBytesRead;
return 0;
}

int http_request_handle(struct mg_connection *connection) {
int ret_status = 400; //Default bad request

Expand Down Expand Up @@ -208,59 +251,34 @@ 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);
size_t bytesRead = 0;
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 {
no_data = true;
}

if (bytes_read > 0 || no_data) {
ret_status = httpSvc->doPost(httpSvc->handle, connection, rcv_buf, (size_t) bytes_read);
} 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);
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 {
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);
size_t bytesRead = 0;
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 {
no_data = true;
}

if (bytes_read > 0 || no_data) {
ret_status = httpSvc->doPut(httpSvc->handle, connection, req_uri, rcv_buf,
(size_t) bytes_read);
} 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);
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 {
ret_status = 0; //Let civetweb handle the request
}
Expand All @@ -272,29 +290,17 @@ 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;
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
}

if (rcv_buf != NULL) {
free(rcv_buf);
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 {
ret_status = 0; //Let civetweb handle the request
}
Expand All @@ -306,30 +312,17 @@ 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;
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
}

if (rcv_buf != NULL) {
free(rcv_buf);
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 {
ret_status = 0; //Let civetweb handle the request
}
Expand Down
3 changes: 3 additions & 0 deletions bundles/remote_services/remote_service_admin_dfi/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Loading