PengZheng commented on code in PR #851:
URL: https://github.com/apache/celix/pull/851#discussion_r4309761703
##########
bundles/http_admin/http_admin/src/http_admin.c:
##########
@@ -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);
Review Comment:
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.
##########
bundles/http_admin/http_admin/src/http_admin.c:
##########
@@ -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);
+ 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) {
Review Comment:
What if `contentLength == -1`? Then we can never receive a request without
explicit `content_length` successfully.
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -518,63 +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);
- 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);
- }
+ 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");
Review Comment:
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.
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -518,63 +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);
- 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);
- }
+ 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) {
Review Comment:
`contentLength == 0` is invalid for our use case.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -151,48 +168,65 @@ 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;
+ 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;
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)) {
Review Comment:
What if `result == -1` with a non-trivial errno? Should we quit?
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -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);
Review Comment:
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.
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -518,63 +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);
- 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);
- }
+ 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);
+ 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));
Review Comment:
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.
##########
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"
Review Comment:
The status texts like "Bad Request"/"server Error" are redundent according
to `mg_send_http_error`
[documentation](https://github.com/civetweb/civetweb/blob/master/docs/api/mg_send_http_error.md):
> 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.
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -518,63 +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);
- 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);
- }
+ if (contentLength > 0 && (unsigned long long)contentLength >
maxRequestBodySize) {
Review Comment:
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.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -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);
Review Comment:
This applies to sockets in non-blocking mode.
I have not checked whether sockets in remote_shell has been set in this mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
