conserver: free correct addrinfo to prevent crash.

[daztucker]

When looping through addrinfo lists in AddrsMatch, keep a copy of the original addrinfo pointers to free instead of ending up at the terminating NULLs and trying to free those.

OpenWRT uses musl in which freeaddrinfo(NULL) is not safe (which is fine, it's not required by the spec) so this fixes a segfault.

Maintainer: @BKPepe
Compile tested: mips_24kc, gl.inet 6416, master
Run tested: mips_24kc, gl.inet 6416, OpenWrt SNAPSHOT, r25153-869df9ecdf with tests below, plus running in "production" on a gl.inet gl-ar150 on 23.05.2.

Description:
This was submitted and accepted upstream as bstansell/conserver#98.
Tests:
root# uname -a
Linux glinet6416 5.15.148 #0 Wed Feb 14 15:22:53 2024 mips GNU/Linux

root# cat /etc/conserver/conserver.cf
config * { }
default full { rw *; }
default * { master google.com; type device; baud 115200; parity none; }
console test1 { master google.com; device /dev/ttyUSB0; }
console test2 { master www.google.com; device /dev/ttyUSB1; }
access * { trusted 127.0.0.1; }

root# /usr/sbin/conserver
[Thu Feb 15 19:41:03 2024] conserver (2162): conserver.com version 8.2.6
[Thu Feb 15 19:41:03 2024] conserver (2162): started as root' by root'
Segmentation fault

root# opkg remove conserver
Removing package conserver from root...
Not deleting modified conffile /etc/conserver/conserver.cf.

root# opkg install /tmp/conserver_8.2.6-3_mips_24kc.ipk
Installing conserver (8.2.6-3) to root...
Configuring conserver.
Collected errors:

• resolve_conffiles: Existing conffile /etc/conserver/conserver.cf is different from the conffile in the new package. The new conffile will be placed at /etc/conserver/conserver.cf-opkg.

root# /usr/sbin/conserver
[Thu Feb 15 19:42:52 2024] conserver (2323): conserver.com version 8.2.6
[Thu Feb 15 19:42:52 2024] conserver (2323): started as root' by root'
[Thu Feb 15 19:42:54 2024] conserver (2323): compare www.google.com and google.com returns 0
[Thu Feb 15 19:42:54 2024] conserver (2323): ERROR: Master(): listen(): Bad file descriptor
[Thu Feb 15 19:42:54 2024] conserver (2323): terminated
(note lack of segfault)

[BKPepe]

Is this patch included in upstream? If not, will you upstream it?

• There is no commit description, who is the author, etc.

[daztucker]

I am the author of the patch. The tests complained about a "dirty patch" which so I removed the commit info thinking that was it, but it wasn't so I put it back. Quoting from the first comment: "This was submitted upstream (bstansell/conserver#98), not yet accepted."

[BKPepe]

Then it should be done differently. Use it from this: https://github.com/bstansell/conserver/pull/98.patch and then refresh it.

[daztucker]

I think I've done that. It's at least past the "dirty patch" check and seems to be building happily.

[daztucker]

Refreshing it (ie "make package/conserver/refresh V=s") drops the patch metadata from the start of the patch?

[daztucker]

I'm confused: are the patches suppose to have the patch metadata before the actual patch or not? And if so, in what format? Some patches in the repo do, but AFAICT (although I'm not 100% sure) all of the times I've tried with that it has failed.

[daztucker]

The patch has now been accepted upstream as bstansell/conserver@47c232b

The test failures seem unrelated to to this change, since it's nowhere near this change, and most other recent PRs are failing the same tests in the same ways.

[BKPepe]

(This repo looks like it is pretty much dead, except for a few users making changes to their "own" packages. There are no one with commit rights doing regular PR reviews anymore. The maintainance is completely arbitrary)

Leave your opinions elsewhere, please. :) I am doing the best what I can do even though doing that in my own free time. If you would like to help, you can. You can raise your objection on the development list, if you are so worried about this dead repository. Checking by recent commits, I disagree about it. Thanks!

If you want to be helpful, please be, otherwise toxic/angry/negative comments are not helpful.

[daztucker]

I see that freeaddrinfo() is used many places in the code. Did you audit all these, or should I?

I checked all the others, and they seem OK. I saw your patch upstream, and while it has the same symptom it's a different pattern. Yours is:

ai = NULL;
if (something) getaddrinfo(... &ai);
freeaddrinfo(ai);

which is unsafe when (something) doesn't happen. Mine is

getaddrinfo(... &ai);
while (ai = ai->ai_next) ...
freeaddrinfo(ai).

In all of the other cases I could see, it already keeps a copy of the original pointer to pass to freeaddrinfo. I guess there could be other NULL-unsafe freeing, but I didn't see it and have not experienced it.