remove unnecessary workaround for fclose() on OpenBSD

[guijan]

Test program:

 #include <sys/utsname.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <fcntl.h>

static int closed;

int
close(int unused)
{
	closed = 1;
	return 0;
}

int main()
{
	struct utsname os;
	int fd;
	FILE *file;
	char *closed_str[] = {" not", ""};
	uname(&os);
	if ((fd = open("/dev/null", O_RDONLY)) == -1)
		return 1;
	if ((file = fdopen(fd, "rb")) == NULL)
		return 1;
	printf("Operating system: %s/%s %s %s\n", os.sysname, os.machine,
	    os.release, os.version);
	closed = 0;
	fclose(stdin);
	printf("close()%s called on stdin\n", closed_str[closed]);
	closed = 0;
	fclose(file);
	printf("close()%s called on nonstd stream\n", closed_str[closed]);
}

Output:

$ ./a.out
Operating system: OpenBSD/amd64 7.4 GENERIC.MP#0
close() not called on stdin
close() not called on nonstd stream

I tried an earlier version of the test program (it only tests stdin) on a bunch of others too to see if it's a thing elsewhere:

$ ./a.out
Operating system: SunOS/i86pc 5.11 omnios-r151046-1c2c17cce7
close() not called.

$ ./a.out
Operating system: Linux/x86_64 6.1.55-0-virt #1-Alpine SMP PREEMPT_DYNAMIC Sun, 24 Sep 2023 23:14:02 +0000
close() not called.

$ ./a.out
Operating system: FreeBSD/amd64 13.2-RELEASE FreeBSD 13.2-RELEASE GENERIC-BWN_GPL_PHY
close() not called.

$ ./a.out
Operating system: NetBSD/i386 9.3 NetBSD 9.3 (GENERIC) #0: Thu Aug  4 15:30:37 UTC 2022  [email protected]:/usr/src/sys/arch/i386/compile/GENERIC
close() not called.

[rofl0r]

i'd like to believe this has been fixed, but the only certain way to test it imo is to set a breakpoint on libc close and then call fclose() - preferably on a FILE* that's been opened with fopen().

//edit: and in case it isn't clear, see whether the breakpoint is hit

//edit2: i just realized we actually had a configure check for this behaviour - which will (or should) make this work for any OpenBSD version - what's the disadvantage of keeping it ? (apart from having a runtime test that's not cross-compile compatible)

[guijan]

Turns out the test in proxychains-ng hasn't worked in 8 years. See this commit: https://cvsweb.openbsd.org/src/lib/libc/stdio/fclose.c?rev=1.10&content-type=text/x-cvsweb-markup

let internal calls resolve directly and
not be overridable

I modified the earlier test program to do nothing but fclose() stdin and a FILE it opened itself:

#include <sys/utsname.h>
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>

int main()
{
	struct utsname os;
	int fd;
	FILE *file;
	char *closed_str[] = {" not", ""};
	uname(&os);
	if ((fd = open("/dev/null", O_RDONLY)) == -1)
		return 1;
	if ((file = fdopen(fd, "rb")) == NULL)
		return 1;
	printf("Operating system: %s/%s %s %s\n", os.sysname, os.machine,
	    os.release, os.version);
	fclose(stdin);
	fclose(file);
}

Then I ran it in gdb with breakpoint in close().

Yes, the code stepped into close() on OpenBSD.

NetBSD fclose() also calls close():

Also, POSIX says:
https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/

The fclose() function shall perform the equivalent of a close() on the file descriptor

I suppose whatever assumption proxychains-ng made need to be fixed, I just noticed the OpenBSD test wasn't detecting anything and didn't look further before making this PR, and now I'm going to bed.

[guijan]

After reading a bit more of the source code, I don't see how whether the underlying fd of a FILE is closed by fclose() has any effect on the code. The only thing the workaround seems to do is leak a FILE on OpenBSD if the compile-time test succeeds.

Inside libproxychains.c's function get_chain_data(), the FILE *file variable is only used 3 times:

file = fopen(...);
fgets(..., file);
#ifndef BROKEN_FCLOSE
fclose(file);
#endif

There's no fdopen() or fileno() or anything of the sort.

I've changed the commit message and force-pushed a new version, the code changes are the same as before.

[rofl0r]

see the original research here #95 and upstream bug report here:
http://marc.info/?l=openbsd-bugs&m=145280872431093&w=2

i'm aware that the current bugfix creates an fd leak, but it was deemed better than crashing.
you still didn't answer the question why the configure check seems like something that needs to be removed.

[guijan]

It's code that isn't doing anything, the close() function that libc calls hasn't been overridable in OpenBSD for a long time.

[rofl0r]

so to conclude, recent openbsd runs the configure check but doesn't activate the hack, whereas old openbsd versions that need the hack properly recognize it and activate it (unless a crosscompiler is used) ?