From 54899b404cbde5a24984e4865eed112f303398f6 Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 24 Dec 2017 22:36:52 +1100 Subject: [PATCH 1/3] Stop invoking undefined behaviour by using tor_free() on an unaligned pointer ... in get_interface_addresses_ioctl(). This pointer alignment issue exists on x86_64 macOS, but is unlikely to exist elsewhere. (i386 macOS only requires 4-byte alignment, and other OSs have 8-byte ints.) Fixes bug 24733; not in any released version of tor. --- changes/bug24733 | 6 ++++++ src/common/address.c | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 changes/bug24733 diff --git a/changes/bug24733 b/changes/bug24733 new file mode 100644 index 0000000000..e333e4fa5d --- /dev/null +++ b/changes/bug24733 @@ -0,0 +1,6 @@ + o Minor bugfixes (code correctness): + - Stop invoking undefined behaviour by using tor_free() on an unaligned + pointer in get_interface_addresses_ioctl(). This pointer alignment issue + exists on x86_64 macOS, but is unlikely to exist elsewhere. + Fixes bug 24733; bugfix on 0.3.0.0-alpha-dev; + not in any released version of tor. diff --git a/src/common/address.c b/src/common/address.c index 0c0ba782ae..ea14e63926 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -1601,7 +1601,11 @@ get_interface_addresses_ioctl(int severity, sa_family_t family) done: if (fd >= 0) close(fd); - tor_free(ifc.ifc_buf); + /* On macOS, tor_free() loads ifc.ifc_buf, which leads to undefined + * behaviour, because it is always aligned at 8-bytes (ifc) plus 4 bytes + * (ifc_len and pragma pack(4)). So we use raw_free() instead. */ + raw_free(ifc.ifc_buf); + ifc.ifc_buf = NULL; return result; } #endif /* defined(HAVE_IFCONF_TO_SMARTLIST) */ From f71bbd20a40de78fc1e7d722ba591578f137abec Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Jan 2018 09:51:45 -0500 Subject: [PATCH 2/3] Extract the raw_free() of ifc_buf into a new function. Explain the problem more correctly. --- src/common/address.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/common/address.c b/src/common/address.c index ea14e63926..d96ec514b1 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -1515,6 +1515,18 @@ get_interface_addresses_win32(int severity, sa_family_t family) #define _SIZEOF_ADDR_IFREQ sizeof #endif +/* Free ifc->ifc_buf safely. */ +static void +ifconf_free_ifc_buf(struct ifconf *ifc) +{ + /* On macOS, tor_free() takes the address of ifc.ifc_buf, which leads to + * undefined behaviour, because pointer-to-pointers are expected to be + * aligned at 8-bytes, but the ifconf structure is packed. So we use + * raw_free() instead. */ + raw_free(ifc->ifc_buf); + ifc->ifc_buf = NULL; +} + /** Convert *buf, an ifreq structure array of size buflen, * into smartlist of tor_addr_t structures. */ @@ -1601,11 +1613,7 @@ get_interface_addresses_ioctl(int severity, sa_family_t family) done: if (fd >= 0) close(fd); - /* On macOS, tor_free() loads ifc.ifc_buf, which leads to undefined - * behaviour, because it is always aligned at 8-bytes (ifc) plus 4 bytes - * (ifc_len and pragma pack(4)). So we use raw_free() instead. */ - raw_free(ifc.ifc_buf); - ifc.ifc_buf = NULL; + ifconf_free_ifc_buf(&ifc); return result; } #endif /* defined(HAVE_IFCONF_TO_SMARTLIST) */ From 519fa1a3e6943f858b5dc1dee461053af1c187cd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Jan 2018 09:55:01 -0500 Subject: [PATCH 3/3] Document the alignment limitation of tor_free() --- src/common/util.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/common/util.h b/src/common/util.h index 8dc64ce9fa..2ee0ea28cd 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -79,6 +79,11 @@ extern int dmalloc_free(const char *file, const int line, void *pnt, * * This is a macro. If you need a function pointer to release memory from * tor_malloc(), use tor_free_(). + * + * Note that this macro takes the address of the pointer it is going to + * free and clear. If that pointer is stored with a nonstandard + * alignment (eg because of a "packed" pragma) it is not correct to use + * tor_free(). */ #ifdef __GNUC__ #define tor_free(p) STMT_BEGIN \