Opened 2 months ago

Last modified 5 weeks ago

#31698 new enhancement

Reconsider HAVE_XXX_H usage in the Tor code

Reported by: ahf Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We currently sometimes have code like:

#ifdef HAVE_STRING_H
#  include <string.h>
#endif

But we don't expect to work on systems that do not have, for example, string.h available. We should not do these check in every .c and .h file, but instead just have our configure script fail if these headers are not available.

Child Tickets

TicketTypeStatusOwnerSummary
#31699defectnewRemove unused configure.ac checks

Attachments (1)

count_include_checks.sh (544 bytes) - added by nickm 2 months ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 2 months ago by nickm

This list might be helpful. It lists every system header we include that has a guard defined in orconfig.h. The first column is the number of times we check HAVE_FOO_H before we include it; the second column is the number of times we include it. I will attach the kludgey script I used to generate it.

0	102	stdlib.h
0	10	event2/event.h
0	1	crt_externs.h
0	1	net/pfvar.h
0	1	sys/select.h
0	2	libscrypt.h
0	2	seccomp.h
0	2	stdatomic.h
0	32	stdint.h
0	3	event2/dns.h
0	3	inttypes.h
1	1	android/log.h
1	1	mach/vm_inherit.h
1	1	malloc.h
1	1	readpassphrase.h
1	1	sys/eventfd.h
1	1	sys/fcntl.h
1	1	sys/limits.h
1	1	syslog.h
1	1	sys/random.h
1	1	sys/statvfs.h
1	1	sys/utime.h
1	2	cygwin/signal.h
1	2	execinfo.h
1	2	grp.h
1	2	sys/file.h
1	2	sys/syscall.h
1	2	sys/sysctl.h
1	2	ucontext.h
1	3	ifaddrs.h
1	3	sys/ucontext.h
2	145	string.h
2	2	crypto_scalarmult_curve25519.h
2	2	linux/netfilter_ipv4.h
2	2	linux/netfilter_ipv6/ip6_tables.h
2	2	nacl/crypto_scalarmult_curve25519.h
2	2	sys/capability.h
2	3	sys/prctl.h
2	3	sys/un.h
2	4	pthread.h
2	6	netdb.h
2	8	limits.h
3	3	linux/if.h
3	3	linux/types.h
3	4	sys/resource.h
3	4	sys/wait.h
3	6	pwd.h
4	17	time.h
4	35	errno.h
4	4	utime.h
4	5	arpa/inet.h
4	6	net/if.h
4	7	sys/mman.h
4	8	signal.h
5	5	netinet/in6.h
5	5	sys/ioctl.h
9	12	sys/param.h
10	10	netinet/in.h
15	16	sys/socket.h
15	18	sys/time.h
22	26	fcntl.h
30	32	sys/stat.h
30	41	sys/types.h
58	65	unistd.h

Changed 2 months ago by nickm

Attachment: count_include_checks.sh added

comment:2 Changed 2 months ago by nickm

(We'll need to hand-check each of these, but it might be a good list to start with.)

comment:3 in reply to:  description Changed 2 months ago by teor

Replying to ahf:

But we don't expect to work on systems that do not have, for example, string.h available. We should not do these check in every .c and .h file, but instead just have our configure script fail if these headers are not available.

How will we know which headers have substitutes, and which headers are actually required?

For example, if a file is only using strlcat/strlcpy from string.h, and the platform doesn't have string.h, we should just use our implementations in compat_string.h. Just like we would for a platform that has string.h, but no strlcat/strlcpy.

comment:4 Changed 5 weeks ago by eighthave

android/log.h is always present when building for Android. No need to test for it, just use it:

#ifdef __ANDROID__
#include <android/log.h>
#endif

And __ANDROID__ is always present, try this with an NDK version since at least r10, probably earlier even:

$ for clang in /opt/android-ndk-r20/toolchains/llvm/prebuilt/linux-x86_64/bin/*-clang; do printf "%s\t" `basename $clang`; $clang -E -dM - < /dev/null | grep __ANDROID__; done
$ for gcc in /opt/android-ndk-r10e/toolchains/*/prebuilt/*/bin/*-gcc; do printf "%s\t" `basename $gcc`; $gcc  -E -dM - < /dev/null | grep __ANDROID__; done
Note: See TracTickets for help on using tickets.