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 2 commits into
base: master
Choose a base branch
from
Open

Improve remote shell loop error handling #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
123 changes: 49 additions & 74 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 @@ -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);
Expand Down Expand Up @@ -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) {

@PengZheng PengZheng Oct 11, 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.

read == 0 usually means EOF, rather than an error.

The function mg_read() receives data over an existing connection. The data is handled as binary and is stored in a buffer whose address has been provided as a parameter. The function returns the number of read bytes when successful, the value 0 when the connection has been closed by peer and a negative value when no more data could be read from the connection.

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

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
71 changes: 42 additions & 29 deletions bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,41 +528,54 @@ static int remoteServiceAdmin_callback(struct mg_connection *conn) {
if (export != NULL) {
uint64_t datalength = request_info->content_length;

@PengZheng PengZheng Oct 11, 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.

According to official documentation:

|content_length|long long| The content length of the request body. This value can be -1 if no content length was provided. The request may still have body data, but the server cannot determine the length until all data has arrived (e.g. when the client closes the connection, or the final chunk of a chunked request has been received). |

If every request body is required to have content_length, then we need to check against -1.
Do we need to check against a reasonable upper bound to avoid allocating too much memory?

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);
Expand Down
8 changes: 6 additions & 2 deletions bundles/shell/remote_shell/src/connection_listener.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {

Expand Down
56 changes: 37 additions & 19 deletions bundles/shell/remote_shell/src/remote_shell.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -172,27 +175,42 @@ 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);
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);
len = recv(fd, buff + used, COMMAND_BUFF_SIZE - 1 - used, 0);

@PengZheng PengZheng Oct 11, 2026 •
edited
Loading

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.

This is still not robust enough. A common error breaking this is EINTR, which requires retry.
Note EINTR is not the only such status code, EAGAIN/EWOULDBLOCK to name a few.
We also need to take platform difference (macOS and Linux) into account.

if (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';

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);
}

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 {
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;
}
}
}
Expand Down
Loading