Improve remote shell loop error handling by pnoltes · Pull Request #851 · apache/celix

pnoltes · 2026-09-27T21:41:38Z

Improve remote shell loop by recv return and then logging result and exiting loop if needed.

codecov-commenter · 2026-09-27T21:57:45Z

Codecov Report

❌ Patch coverage is 53.84615% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.55%. Comparing base (270c784) to head (885c8e4).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...e_service_admin_dfi/src/remote_service_admin_dfi.c	60.67%	35 Missing ⚠️
bundles/http_admin/http_admin/src/http_admin.c	42.59%	31 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #851    +/-   ##
========================================
  Coverage   91.54%   91.55%            
========================================
  Files         235      235            
  Lines       28787    28961   +174     
========================================
+ Hits        26353    26514   +161     
- Misses       2434     2447    +13

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

PengZheng

LGTM

Though networking bundles are not robust enough to be used in production environment, they are still instructive to help programmers learn how to integrate Celix. It's OK that we add some basic protections first before a refactoring.

PengZheng

Nice improvement for our networking code.
The HTTP code is good enough except for its zero return handling.
The TCP part is more difficult to deal with raw socket API.

PengZheng · 2026-10-11T08:45:32Z

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


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.

done, also added a specific function to check EAGAIN/EWOULDBLOCK, so that we can make this platform dependent in a cohesive way.

This applies to sockets in non-blocking mode.
I have not checked whether sockets in remote_shell has been set in this mode.

PengZheng

I left some further remarks, some of which need to be addressed before merging this PR.

PengZheng · 2026-10-19T01:55:32Z

+            }
+            totalBytesRead += (size_t)read;
+        }
+        if (totalBytesRead < length) {


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

PengZheng · 2026-10-19T02:03:24Z

+    *errorMsgOut = NULL;
+    if (contentLength > 0) {
+        size_t length = (contentLength > INT_MAX) ? INT_MAX : (size_t)contentLength;
+        buffer = malloc(length + 1);


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.

PengZheng · 2026-10-19T03:17:32Z

+
+#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"


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.

PengZheng · 2026-10-19T08:38:41Z

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


As already mentioned, mg_send_http_error will add status text corresponding to error code, as in https://github.com/civetweb/civetweb/blob/588860e30721bf5453b0440c390865a8e85dcae5/src/civetweb.c#L4677

This remark applies to all usages of this API.

PengZheng · 2026-10-19T08:59:55Z

-            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) {


Note that 0 is a valid Content-Length value, which just means message body is empty.
This is invalid for the remote service scenario, and thus we can avoid the following memory allocation and return error early.

PengZheng · 2026-10-19T09:08:26Z

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


It does not make sense to send data_response_headers and response differently.
The return value of the former send is not checked at all.

PengZheng · 2026-10-19T09:14:17Z

-                        break;
-                    }
-                }
+    if (contentLength <= 0 && readLen > maxRequestBodySize) {


contentLength == 0 is invalid for our use case.

PengZheng · 2026-10-19T09:28:23Z

 	for (i = 0; i < length; i += 1) {
 		connection_pt connection = celix_arrayList_get(instance->connections, i);
-		connection->threadRunning = false;
+		connection_setThreadRunning(connection, false);


I have found no synchronization between connection processing thread and the calling thread, and thus suspect use-after-free bugs here.
Moreover, we never calling pthread_join on these connection processing thread, these are definitely resource leaks.

PengZheng · 2026-10-19T11:45:27Z

+		} 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)) {


What if result == -1 with a non-trivial errno? Should we quit?

PengZheng · 2026-10-19T12:04:52Z

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


This applies to sockets in non-blocking mode.
I have not checked whether sockets in remote_shell has been set in this mode.

Improve remote shell loop error handling

bfe6ffb

PengZheng approved these changes Sep 28, 2026

View reviewed changes

Comment thread bundles/shell/remote_shell/src/remote_shell.c Outdated

Improve read loops for http admin, rsa and remote shell

0b909e6

PengZheng reviewed Oct 11, 2026

View reviewed changes

pnoltes added 2 commits October 18, 2026 18:49

Improve err handling rsa and add max request body size for rsa dfi

dd06cb4

Improve remote shell recv handling

885c8e4

PengZheng requested changes Oct 19, 2026

View reviewed changes

Conversation

pnoltes commented Sep 27, 2026

Uh oh!

codecov-commenter commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PengZheng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PengZheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PengZheng Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PengZheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Sep 27, 2026 •

edited

Loading

PengZheng left a comment •

edited

Loading

PengZheng Oct 11, 2026 •

edited

Loading