Opened 14 months ago

Last modified 13 months 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
#31699defectneeds_revisionRemove unused configure.ac checks

Attachments (1)

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

Download all attachments as: .zip

Change History (5)

comment:1 Changed 14 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 14 months ago by nickm

Attachment: count_include_checks.sh added

comment:2 Changed 14 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 13 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 13 months 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.