stephane
modbus_new_tcp_pi allocates way more memory than modbus_new_tcp and has arbitrary length limits on its string arguments
Hi!
libmodbus version
Sure, but I am not using any version:
$ pkg-config --modversion libmodbus
Package libmodbus was not found in the pkg-config search path.
Perhaps you should add the directory containing `libmodbus.pc'
to the PKG_CONFIG_PATH environment variable
Package 'libmodbus', required by 'virtual:world', not found
OS and/or distribution
$ cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux bookworm/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"
Environment
Since you apparently want to know my architecture, here's my CPU:
Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
Description
The "protocol independent" TCP code uses this struct:
https://github.com/stephane/libmodbus/blob/3da2d01916ef118aba30a1951d89e0ca0f90e2f7/src/modbus-tcp-private.h#L30-L42
This has arbitrary length limits of 1024 bytes of the node length and 31 bytes for the service.
Also, this allocates more than 1 KiB of memory even when I just provide 10 characters. That is 99% wasted memory for storing two short strings.
The implementation returns an error if these limits are reached:
https://github.com/stephane/libmodbus/blob/3da2d01916ef118aba30a1951d89e0ca0f90e2f7/src/modbus-tcp.c#L896-L897
https://github.com/stephane/libmodbus/blob/3da2d01916ef118aba30a1951d89e0ca0f90e2f7/src/modbus-tcp.c#L919-L920
Expected behaviour
Less memory usage and no arbitrary limits.
Actual behaviour
This section doesn't really fit this report, so I will instead use this for suggestions.
I see two ways for this:
- Instead of using a fixed size array, do an extra memory allocation. E.g. (change is completely untested and should only show the idea):
diff --git a/src/modbus-tcp-private.h b/src/modbus-tcp-private.h
index 164c8c7..adef8a9 100644
--- a/src/modbus-tcp-private.h
+++ b/src/modbus-tcp-private.h
@@ -27,18 +27,15 @@ typedef struct _modbus_tcp {
char ip[16];
} modbus_tcp_t;
-#define _MODBUS_TCP_PI_NODE_LENGTH 1025
-#define _MODBUS_TCP_PI_SERVICE_LENGTH 32
-
typedef struct _modbus_tcp_pi {
/* Transaction ID */
uint16_t t_id;
/* TCP port */
int port;
/* Node */
- char node[_MODBUS_TCP_PI_NODE_LENGTH];
+ char* node;
/* Service */
- char service[_MODBUS_TCP_PI_SERVICE_LENGTH];
+ char* service;
} modbus_tcp_pi_t;
#endif /* MODBUS_TCP_PRIVATE_H */
diff --git a/src/modbus-tcp.c b/src/modbus-tcp.c
index 5164273..47aaeca 100644
--- a/src/modbus-tcp.c
+++ b/src/modbus-tcp.c
@@ -744,6 +744,14 @@ static void _modbus_tcp_free(modbus_t *ctx) {
free(ctx);
}
+static void _modbus_tcp_pi_free(modbus_t *ctx) {
+ modbus_tcp_pi_t *ctx_tcp_pi = ctx->backend_data;
+ free(ctx_tcp_pi->node);
+ free(ctx_tcp_pi->service);
+ free(ctx->backend_data);
+ free(ctx);
+}
+
const modbus_backend_t _modbus_tcp_backend = {
_MODBUS_BACKEND_TYPE_TCP,
_MODBUS_TCP_HEADER_LENGTH,
@@ -786,7 +794,7 @@ const modbus_backend_t _modbus_tcp_pi_backend = {
_modbus_tcp_close,
_modbus_tcp_flush,
_modbus_tcp_select,
- _modbus_tcp_free
+ _modbus_tcp_pi_free
};
modbus_t* modbus_new_tcp(const char *ip, int port)
@@ -858,8 +866,6 @@ modbus_t* modbus_new_tcp_pi(const char *node, const char *service)
{
modbus_t *ctx;
modbus_tcp_pi_t *ctx_tcp_pi;
- size_t dest_size;
- size_t ret_size;
ctx = (modbus_t *)malloc(sizeof(modbus_t));
if (ctx == NULL) {
@@ -880,49 +886,34 @@ modbus_t* modbus_new_tcp_pi(const char *node, const char *service)
}
ctx_tcp_pi = (modbus_tcp_pi_t *)ctx->backend_data;
+ /* Ensure this variable is initialised for proper error handling below */
+ ctx_tcp_pi->service = NULL;
+
if (node == NULL) {
/* The node argument can be empty to indicate any hosts */
- ctx_tcp_pi->node[0] = 0;
- } else {
- dest_size = sizeof(char) * _MODBUS_TCP_PI_NODE_LENGTH;
- ret_size = strlcpy(ctx_tcp_pi->node, node, dest_size);
- if (ret_size == 0) {
- fprintf(stderr, "The node string is empty\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
+ node = "";
+ }
+ ctx_tcp_pi->node = strdup(node);
+ if (ctx_tcp_pi->node == NULL) {
+ modbus_free(ctx);
+ errno = ENOMEM;
+ return NULL;
+ }
- if (ret_size >= dest_size) {
- fprintf(stderr, "The node string has been truncated\n");
+ if (service != NULL && service[0] != '\0') {
+ ctx_tcp_pi->service = strdup(service);
+ if (ctx_tcp_pi->service == NULL) {
modbus_free(ctx);
- errno = EINVAL;
+ errno = ENOMEM;
return NULL;
}
- }
-
- if (service != NULL) {
- dest_size = sizeof(char) * _MODBUS_TCP_PI_SERVICE_LENGTH;
- ret_size = strlcpy(ctx_tcp_pi->service, service, dest_size);
} else {
- /* Empty service is not allowed, error caught below. */
- ret_size = 0;
- }
-
- if (ret_size == 0) {
fprintf(stderr, "The service string is empty\n");
modbus_free(ctx);
errno = EINVAL;
return NULL;
}
- if (ret_size >= dest_size) {
- fprintf(stderr, "The service string has been truncated\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
-
ctx_tcp_pi->t_id = 0;
return ctx;
If you do not like the two extra memory allocations:
My other idea would be to use flexible array members. This means something like that (patch also completely untested):
diff --git a/src/modbus-tcp-private.h b/src/modbus-tcp-private.h
index 164c8c7..9a54c13 100644
--- a/src/modbus-tcp-private.h
+++ b/src/modbus-tcp-private.h
@@ -35,10 +35,8 @@ typedef struct _modbus_tcp_pi {
uint16_t t_id;
/* TCP port */
int port;
- /* Node */
- char node[_MODBUS_TCP_PI_NODE_LENGTH];
- /* Service */
- char service[_MODBUS_TCP_PI_SERVICE_LENGTH];
+ /* Node and service concatenated into one string */
+ char node_and_service[];
} modbus_tcp_pi_t;
#endif /* MODBUS_TCP_PRIVATE_H */
diff --git a/src/modbus-tcp.c b/src/modbus-tcp.c
index 5164273..3593127 100644
--- a/src/modbus-tcp.c
+++ b/src/modbus-tcp.c
@@ -358,6 +358,8 @@ static int _modbus_tcp_pi_connect(modbus_t *ctx)
struct addrinfo *ai_list;
struct addrinfo *ai_ptr;
struct addrinfo ai_hints;
+ const char *node;
+ const char *service;
modbus_tcp_pi_t *ctx_tcp_pi = ctx->backend_data;
#ifdef OS_WIN32
@@ -366,6 +368,9 @@ static int _modbus_tcp_pi_connect(modbus_t *ctx)
}
#endif
+ node = ctx_tcp_pi->node_and_service;
+ service = ctx_tcp_pi->node_and_service + strlen(node) + 1;
+
memset(&ai_hints, 0, sizeof(ai_hints));
#ifdef AI_ADDRCONFIG
ai_hints.ai_flags |= AI_ADDRCONFIG;
@@ -377,8 +382,7 @@ static int _modbus_tcp_pi_connect(modbus_t *ctx)
ai_hints.ai_next = NULL;
ai_list = NULL;
- rc = getaddrinfo(ctx_tcp_pi->node, ctx_tcp_pi->service,
- &ai_hints, &ai_list);
+ rc = getaddrinfo(node, service, &ai_hints, &ai_list);
if (rc != 0) {
if (ctx->debug) {
fprintf(stderr, "Error returned by getaddrinfo: %s\n", gai_strerror(rc));
@@ -564,6 +568,8 @@ int modbus_tcp_pi_listen(modbus_t *ctx, int nb_connection)
}
#endif
+ // TODO: Handle changed struct
+
if (ctx_tcp_pi->node[0] == 0) {
node = NULL; /* == any */
} else {
@@ -858,10 +864,27 @@ modbus_t* modbus_new_tcp_pi(const char *node, const char *service)
{
modbus_t *ctx;
modbus_tcp_pi_t *ctx_tcp_pi;
- size_t dest_size;
- size_t ret_size;
+ size_t node_size;
+ size_t service_size;
- ctx = (modbus_t *)malloc(sizeof(modbus_t));
+ if (node == NULL) {
+ /* The node argument can be empty to indicate any hosts */
+ node_size = 0;
+ node = "";
+ } else {
+ node_size = strlen(node_size);
+ }
+
+ if (service != NULL && service[0] != '\0') {
+ service_size = strlen(service);
+ } else {
+ fprintf(stderr, "The service string is empty\n");
+ modbus_free(ctx);
+ errno = EINVAL;
+ return NULL;
+ }
+
+ ctx = (modbus_t *)malloc(sizeof(modbus_t) + node_size + 1 + service_size + 1);
if (ctx == NULL) {
return NULL;
}
@@ -880,48 +903,8 @@ modbus_t* modbus_new_tcp_pi(const char *node, const char *service)
}
ctx_tcp_pi = (modbus_tcp_pi_t *)ctx->backend_data;
- if (node == NULL) {
- /* The node argument can be empty to indicate any hosts */
- ctx_tcp_pi->node[0] = 0;
- } else {
- dest_size = sizeof(char) * _MODBUS_TCP_PI_NODE_LENGTH;
- ret_size = strlcpy(ctx_tcp_pi->node, node, dest_size);
- if (ret_size == 0) {
- fprintf(stderr, "The node string is empty\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
-
- if (ret_size >= dest_size) {
- fprintf(stderr, "The node string has been truncated\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
- }
-
- if (service != NULL) {
- dest_size = sizeof(char) * _MODBUS_TCP_PI_SERVICE_LENGTH;
- ret_size = strlcpy(ctx_tcp_pi->service, service, dest_size);
- } else {
- /* Empty service is not allowed, error caught below. */
- ret_size = 0;
- }
-
- if (ret_size == 0) {
- fprintf(stderr, "The service string is empty\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
-
- if (ret_size >= dest_size) {
- fprintf(stderr, "The service string has been truncated\n");
- modbus_free(ctx);
- errno = EINVAL;
- return NULL;
- }
+ strcpy(ctx_tcp_pi->node_and_service, node);
+ strcpy(&ctx_tcp_pi->node_and_service[node_size + 1], service);
ctx_tcp_pi->t_id = 0;
Steps to reproduce the behavior (commands or source code)
Source code is linked above, I guess. I don't have any own code using libmodbus, so....?
libmodbus output with debug mode enabled
Uhm... command not found, I guess? See pkg-config output from above.
Copyright
If you do not worry about copyright issues with my untested sketch patches above, then just ignore this section.
In case you worry: Pick one of the following:
- If you are living in a jurisdiction were this works (I do not): I hereby release all information in this post to the public domain.
- I hereby license all information in this post under the WTFPLv2. See https://en.wikipedia.org/wiki/WTFPL. I explicitly mention that I think this allows to act as if you own all copyright: You can rewrite, change the license, sell, and whatever else you want.
LGTM.
Which of the two proposed variants do you mean? Or are both fine?
Could you provide a PR, please?
Sorry, nope, I cannot. I would gladly provide a LGPL licensed patch, but you do not accept such contributions. In fact, I fear that if I actually work too much on this, the license would actually prevent anyone else from doing the work in a way that you would deem acceptable (because that would then easily end up as a derived work of my patch and that makes license stuff more complicated).
I choose the proposal with 2 mallocs, way easier to understand and maintain ;) Thank you @psychon