From 05610d1564c9ea75d7e10780e2c0f8973e5795ab Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 14 Sep 2023 22:31:26 +0200 Subject: [PATCH 1/6] common/common.c: extend xbasename(), xstrdup() and xrealloc() definition to return NULL if input was NULL (and log it) [#2052] Signed-off-by: Jim Klimov --- common/common.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/common/common.c b/common/common.c index 0864f28b23..3949dbd016 100644 --- a/common/common.c +++ b/common/common.c @@ -643,11 +643,21 @@ int sendsignal(const char *progname, const char * sig) const char *xbasename(const char *file) { + const char *p; +#ifdef WIN32 + const char *r; +#endif + + if (file == NULL) { + upsdebugx(1, "%s: got null input", __func__); + return NULL; + } + #ifndef WIN32 - const char *p = strrchr(file, '/'); + p = strrchr(file, '/'); #else - const char *p = strrchr(file, '\\'); - const char *r = strrchr(file, '/'); + p = strrchr(file, '\\'); + r = strrchr(file, '/'); /* if not found, try '/' */ if( r > p ) { p = r; @@ -1729,7 +1739,14 @@ void *xcalloc(size_t number, size_t size) void *xrealloc(void *ptr, size_t size) { - void *p = realloc(ptr, size); + void *p; + + if (ptr == NULL) { + upsdebugx(1, "%s: got null input", __func__); + return NULL; + } + + p = realloc(ptr, size); if (p == NULL) fatal_with_errno(EXIT_FAILURE, "%s", oom_msg); @@ -1738,7 +1755,14 @@ void *xrealloc(void *ptr, size_t size) char *xstrdup(const char *string) { - char *p = strdup(string); + char *p; + + if (string == NULL) { + upsdebugx(1, "%s: got null input", __func__); + return NULL; + } + + p = strdup(string); if (p == NULL) fatal_with_errno(EXIT_FAILURE, "%s", oom_msg); From fde7672eff2a2f9b50f5d3353e89e70712f3554f Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 14 Sep 2023 22:39:15 +0200 Subject: [PATCH 2/6] clients/upsclient.c: fix strdup()=>xstrdup() to not segfault with bad inputs [#2052] Signed-off-by: Jim Klimov --- clients/upsclient.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clients/upsclient.c b/clients/upsclient.c index 6a823fa57c..f6dea627a5 100644 --- a/clients/upsclient.c +++ b/clients/upsclient.c @@ -1161,7 +1161,7 @@ int upscli_tryconnect(UPSCONN_t *ups, const char *host, uint16_t port, int flags pconf_init(&ups->pc_ctx, NULL); - ups->host = strdup(host); + ups->host = xstrdup(host); if (!ups->host) { ups->upserror = UPSCLI_ERR_NOMEM; @@ -1618,15 +1618,15 @@ int upscli_splitname(const char *buf, char **upsname, char **hostname, uint16_t s = strchr(tmp, '@'); - if ((*upsname = strdup(strtok_r(tmp, "@", &last))) == NULL) { - fprintf(stderr, "upscli_splitname: strdup failed\n"); + if ((*upsname = xstrdup(strtok_r(tmp, "@", &last))) == NULL) { + fprintf(stderr, "upscli_splitname: xstrdup failed\n"); return -1; } /* only a upsname is specified, fill in defaults */ if (s == NULL) { - if ((*hostname = strdup("localhost")) == NULL) { - fprintf(stderr, "upscli_splitname: strdup failed\n"); + if ((*hostname = xstrdup("localhost")) == NULL) { + fprintf(stderr, "upscli_splitname: xstrdup failed\n"); return -1; } @@ -1659,8 +1659,8 @@ int upscli_splitaddr(const char *buf, char **hostname, uint16_t *port) return -1; } - if ((*hostname = strdup(strtok_r(tmp+1, "]", &last))) == NULL) { - fprintf(stderr, "upscli_splitaddr: strdup failed\n"); + if ((*hostname = xstrdup(strtok_r(tmp+1, "]", &last))) == NULL) { + fprintf(stderr, "upscli_splitaddr: xstrdup failed\n"); return -1; } @@ -1672,8 +1672,8 @@ int upscli_splitaddr(const char *buf, char **hostname, uint16_t *port) } else { s = strchr(tmp, ':'); - if ((*hostname = strdup(strtok_r(tmp, ":", &last))) == NULL) { - fprintf(stderr, "upscli_splitaddr: strdup failed\n"); + if ((*hostname = xstrdup(strtok_r(tmp, ":", &last))) == NULL) { + fprintf(stderr, "upscli_splitaddr: xstrdup failed\n"); return -1; } From 048876f33f3f405c54b30db6f6ab59c5b6b32872 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 14 Sep 2023 22:43:08 +0200 Subject: [PATCH 3/6] NEWS.adoc: update for issue #2052 fix Signed-off-by: Jim Klimov --- NEWS.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index 171eb2349b..dbef0108fa 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -114,6 +114,10 @@ as part of https://github.com/networkupstools/nut/issues/1410 solution. seems to have confused the MGE SHUT (Serial HID UPS Transfer) driver support [#2022] + - An issue was identified which could cause `libupsclient` parser of device + and host names to crash upon bad inputs (e.g. poorly resolved environment + variables in scripts). Now it should fail more gracefully [#2052] + - New `configure --enable-inplace-runtime` option should set default values for `--sysconfdir`, `--with-user` and `--with-group` options to match an existing NUT deployment -- for users who are trying if a custom build From d2857f693c8447034818672c171beb59c12e18d2 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 14 Sep 2023 23:28:04 +0200 Subject: [PATCH 4/6] clients/upsclient.c: upscli_splitname(): add explicit checks for empty upsname and/or hostname[:port] parts, to report problems as such [#2052] Signed-off-by: Jim Klimov --- clients/upsclient.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/clients/upsclient.c b/clients/upsclient.c index f6dea627a5..cd30fedf67 100644 --- a/clients/upsclient.c +++ b/clients/upsclient.c @@ -1618,11 +1618,28 @@ int upscli_splitname(const char *buf, char **upsname, char **hostname, uint16_t s = strchr(tmp, '@'); + /* someone passed a "@hostname" string? */ + if (s == tmp) { + fprintf(stderr, "upscli_splitname: got empty upsname string\n"); + return -1; + } + if ((*upsname = xstrdup(strtok_r(tmp, "@", &last))) == NULL) { fprintf(stderr, "upscli_splitname: xstrdup failed\n"); return -1; } + /* someone passed a "@hostname" string (take two)? */ + if (!**upsname) { + fprintf(stderr, "upscli_splitname: got empty upsname string\n"); + return -1; + } + + /* + fprintf(stderr, "upscli_splitname3: got buf='%s', tmp='%s', upsname='%s', possible hostname:port='%s'\n", + NUT_STRARG(buf), NUT_STRARG(tmp), NUT_STRARG(*upsname), NUT_STRARG((s ? s+1 : s))); + */ + /* only a upsname is specified, fill in defaults */ if (s == NULL) { if ((*hostname = xstrdup("localhost")) == NULL) { @@ -1634,6 +1651,12 @@ int upscli_splitname(const char *buf, char **upsname, char **hostname, uint16_t return 0; } + /* someone passed a "upsname@" string? */ + if (!(*(s+1))) { + fprintf(stderr, "upscli_splitname: got the @ separator and then an empty hostname[:port] string\n"); + return -1; + } + return upscli_splitaddr(s+1, hostname, port); } From 05edad73910a9b0cd59c732b94c303bfe15ea688 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 16 Sep 2023 18:18:38 +0200 Subject: [PATCH 5/6] Revert "common/common.c: extend xbasename(), xstrdup() and xrealloc() definition to return NULL if input was NULL (and log it) [#2052]" This reverts commit 05610d1564c9ea75d7e10780e2c0f8973e5795ab. Seems to cause segfaults on its own, maybe something relied on older behavior (non-NULLs returned in case of bad inputs?) To investigate separately later... --- common/common.c | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/common/common.c b/common/common.c index 3949dbd016..0864f28b23 100644 --- a/common/common.c +++ b/common/common.c @@ -643,21 +643,11 @@ int sendsignal(const char *progname, const char * sig) const char *xbasename(const char *file) { - const char *p; -#ifdef WIN32 - const char *r; -#endif - - if (file == NULL) { - upsdebugx(1, "%s: got null input", __func__); - return NULL; - } - #ifndef WIN32 - p = strrchr(file, '/'); + const char *p = strrchr(file, '/'); #else - p = strrchr(file, '\\'); - r = strrchr(file, '/'); + const char *p = strrchr(file, '\\'); + const char *r = strrchr(file, '/'); /* if not found, try '/' */ if( r > p ) { p = r; @@ -1739,14 +1729,7 @@ void *xcalloc(size_t number, size_t size) void *xrealloc(void *ptr, size_t size) { - void *p; - - if (ptr == NULL) { - upsdebugx(1, "%s: got null input", __func__); - return NULL; - } - - p = realloc(ptr, size); + void *p = realloc(ptr, size); if (p == NULL) fatal_with_errno(EXIT_FAILURE, "%s", oom_msg); @@ -1755,14 +1738,7 @@ void *xrealloc(void *ptr, size_t size) char *xstrdup(const char *string) { - char *p; - - if (string == NULL) { - upsdebugx(1, "%s: got null input", __func__); - return NULL; - } - - p = strdup(string); + char *p = strdup(string); if (p == NULL) fatal_with_errno(EXIT_FAILURE, "%s", oom_msg); From 7c59cf182e0a4eb35ddcc09488180959401838b0 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 16 Sep 2023 18:22:06 +0200 Subject: [PATCH 6/6] common/common.c: extend xstrdup() definition to return NULL if input was NULL (and log it) [#2052] Signed-off-by: Jim Klimov --- common/common.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common/common.c b/common/common.c index 0864f28b23..f96584db60 100644 --- a/common/common.c +++ b/common/common.c @@ -1738,7 +1738,14 @@ void *xrealloc(void *ptr, size_t size) char *xstrdup(const char *string) { - char *p = strdup(string); + char *p; + + if (string == NULL) { + upsdebugx(1, "%s: got null input", __func__); + return NULL; + } + + p = strdup(string); if (p == NULL) fatal_with_errno(EXIT_FAILURE, "%s", oom_msg);