Fix memory leaks in syslogd

[trix_juniper.net]

Summary

syslogd.c main,allowaddr
Free memory assigned to fdsr before dieing.
Free memory assigned to sp before returning early.

Test Plan

Use clang's static analyzer, scan-build, to find the problem and later to show resolution.

[ngie]

I don't like the approach in the second half of the diff -- there's unnecessary duplication in this change and the prior change. I'd prefer something like this, using a failure goto label and a single set of free calls:

$ git diff usr.sbin/syslogd/syslogd.c
diff --git a/usr.sbin/syslogd/syslogd.c b/usr.sbin/syslogd/syslogd.c
index b4c5bcf154c4..90d56025fd32 100644
--- a/usr.sbin/syslogd/syslogd.c
+++ b/usr.sbin/syslogd/syslogd.c
@@ -2451,6 +2451,8 @@ allowaddr(char *s)
        if (ap == NULL)
                err(1, "malloc failed");

+       res = NULL;
+
 #ifdef INET6
        if (*s != '[' || (cp1 = strchr(s + 1, ']')) == NULL)
 #endif
@@ -2466,7 +2468,7 @@ allowaddr(char *s)
                } else {
                        ap->port = strtol(cp1, &cp2, 0);
                        if (*cp2 != '\0')
-                               return (-1); /* port not numeric */
+                               goto fail; /* port not numeric */
                }
        } else {
                if ((se = getservbyname("syslog", "udp")))
@@ -2480,7 +2482,7 @@ allowaddr(char *s)
            strspn(cp1 + 1, "0123456789") == strlen(cp1 + 1)) {
                *cp1 = '\0';
                if ((masklen = atoi(cp1 + 1)) < 0)
-                       return (-1);
+                       goto fail;
        }
 #ifdef INET6
        if (*s == '[') {
@@ -2525,20 +2527,16 @@ allowaddr(char *s)
                        } else if (masklen <= 32) {
                                /* convert masklen to netmask */
                                *maskp = htonl(~((1 << (32 - masklen)) - 1));
-                       } else {
-                               freeaddrinfo(res);
-                               return (-1);
-                       }
+                       } else
+                               goto fail;
                        /* Lose any host bits in the network number. */
                        *addrp &= *maskp;
                        break;
 #endif
 #ifdef INET6
                case AF_INET6:
-                       if (masklen > 128) {
-                               freeaddrinfo(res);
-                               return (-1);
-                       }
+                       if (masklen > 128)
+                               goto fail;
                        if (masklen < 0)
                                masklen = 128;
                        mask6p = (uint32_t *)&sstosin6(&ap->a_mask)->sin6_addr.s6_addr32[0];
@@ -2559,8 +2557,7 @@ allowaddr(char *s)
                        break;
 #endif
                default:
-                       freeaddrinfo(res);
-                       return (-1);
+                       goto fail;
                }
                freeaddrinfo(res);
        } else {
@@ -2597,6 +2594,12 @@ allowaddr(char *s)
        }
 #endif
        return (0);
+fail:
+       if (res != NULL)
+               freeaddrinfo(res);
+       free(ap);
+
+       return (-1);
 }

 /*

usr.sbin/syslogd/syslogd.c

689 ↗

(On Diff #26259)

This doesn't give us much value, given that die is non-reentrant.

die(..) on the other hand is doing wayyyyyy too much. It should be setting a flag, exiting the loop, then free'ing after the function is done.

[trix_juniper.net]

I¹ll take care of this tomorrow.

[trix_juniper.net]

Collect the error handling in allowaddr to the 'err' label

[ngie]

usr.sbin/syslogd/syslogd.c

2601 ↗

(On Diff #26280)

I vote to not add end because it obfuscates the execution flow a little bit.

[ngie]

usr.sbin/syslogd/syslogd.c

2601 ↗

(On Diff #26280)

Also, I understand the idea of saving ret in a variable, but given that it's only set to 0 at the bottom, I don't understand the need for creating a bit more trivial complexity. That's why I hardcoded 0/-1 -- plus the diff was slightly smaller, so it was easier to backport, in theory... I'll have to look at it a bit further, but the function (and surrounding commits) may not have been MFCed yet.

[trix_juniper.net]

This conflicts with earlier comment to have goto.
This is the pattern I use to have a single return, if you want 2 returns,
I could remove the Œret¹ and Œend:¹ and return(-1) at the bottom of the
err: block.

[trix_juniper.net]

remove 'end' label and 'ret' variable

[ngie]

Thank you very much, and sorry for being a nitnicky Nancy about this! I just want to make things more straightforward because syslogd(8) has a sorted past in terms of complexity.

[trix_juniper.net]

No worries.
Thanks for the commit!
Tom