modbus_new_tcp_pi allocates way more memory than modbus_new_tcp and has arbitrary length limits on its string arguments

[psychon]

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.

[stephane]

LGTM. Could you provide a PR, please?

[psychon]

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

[stephane]

I choose the proposal with 2 mallocs, way easier to understand and maintain ;) Thank you @psychon

[psychon]

Thanks!