[Cluster-devel] [PATCHv2 dlm-tool 1/2] dlm_controld: be sure we close logging at last

[Cluster-devel] [PATCHv2 dlm-tool 1/2] dlm_controld: be sure we close logging at last

[Alexander Aring]

I currently try to debug the following:

(gdb) bt
0  _int_malloc (av=av at entry=0x7fd353ac2bc0 <main_arena>, bytes=bytes at entry=8192) at malloc.c:3755
1  0x00007fd3537a04a6 in __libc_calloc (n=n at entry=1, elem_size=elem_size at entry=8192) at malloc.c:3445
2  0x00007fd353792bd7 in __GI___open_memstream (bufloc=bufloc at entry=0x7ffc4edd2ea0, sizeloc=sizeloc at entry=0x7ffc4edd2ea8) at memstream.c:83
3  0x00007fd35382cba4 in __GI___vsyslog_chk (pri=163, flag=1, fmt=0x5560190522da "%s", ap=0x7ffc4edd2f90) at ../misc/syslog.c:167
4  0x00007fd35382d1e3 in __syslog_chk (pri=pri at entry=3, flag=flag at entry=1, fmt=fmt at entry=0x5560190522da "%s") at ../misc/syslog.c:129
5  0x000055601904e114 in syslog (__fmt=0x5560190522da "%s", __pri=3) at /usr/include/bits/syslog.h:31
6  log_level (name_in=<optimized out>, level_in=<optimized out>, fmt=0x55601905243e "abandoned lockspace %s") at logging.c:166
7  0x000055601903a91e in loop () at main.c:1597
8  main (argc=<optimized out>, argv=<optimized out>) at main.c:2161

We see that the last thing in dlm_controld was log_level() then it
crashed internal handling of libc and syslog().

(gdb) f 6
6  log_level (name_in=<optimized out>, level_in=<optimized out>, fmt=0x55601905243e "abandoned lockspace %s") at logging.c:166
166                     syslog(level, "%s", log_str);

We see that log_level() was called with a format string of "abandoned
lockspace %s" and we only do that after leaving the main loop,
dlm_controld was going to shutdown and crashed.

The reason is that at this time the syslog logging was already closed by
closelog() and we still tried to call syslog() and libc doesn't like it.
We should be sure closing the log functionality is the last thing to do
when exiting dlm_controld. This patch is doing that so that dlm_controld
should not crash anymore.

Reported-by: Barry Marson <bmarson@redhat.com>
---
v2:
 - remove sob.

 dlm_controld/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlm_controld/main.c b/dlm_controld/main.c
index 80fe14bd..7cf6348e 100644
--- a/dlm_controld/main.c
+++ b/dlm_controld/main.c
@@ -1604,12 +1604,14 @@ static int loop(void)
 	close_plocks();
 	close_cpg_daemon();
 	clear_configfs();
-	close_logging();
 	close_cluster();
 	close_cluster_cfg();
 
 	list_for_each_entry(ls, &lockspaces, list)
 		log_error("abandoned lockspace %s", ls->name);
+
+	/* must be end */
+	close_logging();
 	return rv;
 }
 
-- 
2.31.1

[Cluster-devel] [PATCHv2 dlm-tool 2/2] dlm_controld: fix rare off by one

[Alexander Aring]

While debugging I came across a rare off by one when the snprintf()
filled string _exactly_ matched the size (with '\0') and we return the
bytes written without \0. We will then write a "\n\0" pattern at the
end but when the string exactly matched there is missing byte in the
calculation of the "\n\0" pattern because the return value only reduced
the size by one. To fix that we substract -1 from the return value of
snprintf() to have at the end two bytes for the "\n\0" pattern. If we
would hit the case that the buffer exactly matched we truncate the
string by one byte because we need to fit '\n' and '\0' into the buffer.
---
v2:
 - remove sob.
 - only really do the truncate of one byte when the buffer would exact
   match which is the given size and the returned size + 1 ('\0').

 dlm_controld/logging.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/dlm_controld/logging.c b/dlm_controld/logging.c
index 2c57138c..bfd7d274 100644
--- a/dlm_controld/logging.c
+++ b/dlm_controld/logging.c
@@ -181,10 +181,14 @@ void log_level(char *name_in, uint32_t level_in, const char *fmt, ...)
 	ret = vsnprintf(log_str + pos, len - pos, fmt, ap);
 	va_end(ap);
 
-	if (ret >= len - pos)
+	if (ret >= len - pos) {
 		pos = len - 1;
-	else
-		pos += ret;
+	} else {
+		if (ret + 1 == len - pos)
+			pos += ret - 1;
+		else
+			pos += ret;
+	}
 
 	log_str[pos++] = '\n';
 	log_str[pos++] = '\0';
-- 
2.31.1

[Cluster-devel] [PATCHv2 dlm-tool 2/2] dlm_controld: fix rare off by one

[Alexander Aring]

Hi,

On Wed, Oct 5, 2022 at 3:23 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> While debugging I came across a rare off by one when the snprintf()
> filled string _exactly_ matched the size (with '\0') and we return the
> bytes written without \0. We will then write a "\n\0" pattern at the
> end but when the string exactly matched there is missing byte in the
> calculation of the "\n\0" pattern because the return value only reduced
> the size by one. To fix that we substract -1 from the return value of
> snprintf() to have at the end two bytes for the "\n\0" pattern. If we
> would hit the case that the buffer exactly matched we truncate the
> string by one byte because we need to fit '\n' and '\0' into the buffer.
> ---
> v2:
>  - remove sob.
>  - only really do the truncate of one byte when the buffer would exact
>    match which is the given size and the returned size + 1 ('\0').
>
>  dlm_controld/logging.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/dlm_controld/logging.c b/dlm_controld/logging.c
> index 2c57138c..bfd7d274 100644
> --- a/dlm_controld/logging.c
> +++ b/dlm_controld/logging.c
> @@ -181,10 +181,14 @@ void log_level(char *name_in, uint32_t level_in, const char *fmt, ...)
>         ret = vsnprintf(log_str + pos, len - pos, fmt, ap);
>         va_end(ap);
>
> -       if (ret >= len - pos)
> +       if (ret >= len - pos) {
>                 pos = len - 1;
> -       else
> -               pos += ret;
> +       } else {
> +               if (ret + 1 == len - pos)
> +                       pos += ret - 1;

I will drop this patch, this case is being handled in the parent if
branch and sets len-1 which truncates the string...

- Alex