From 44ebecaaed17b55a4429ac137036c9894b0dd730 Mon Sep 17 00:00:00 2001 From: Douglas Katzman Date: Fri, 20 Oct 2017 16:35:56 -0400 Subject: [PATCH] Fixes for running with undefined-behavior sanitizer - Terminate iteration of load_core_file() loop before remaining_len transitions to a "negative" (large positive) value. (It's an os_vm_size_t so this is flagged as causing wraparound on addition) - Use memcpy() for all unaligned loads/stores. C compilers optimize that into a single instruction where possible. - Spell 1 as 1U whenever it might get shifted leftward by 31 bits. - Don't left-shift a negative number ever. --- src/runtime/coreparse.c | 17 +++++++---------- src/runtime/hopscotch.c | 4 ++-- src/runtime/immobile-space.c | 35 +++++++++++++++++++++-------------- src/runtime/runtime.h | 2 +- src/runtime/unaligned.h | 31 +++++++++++++++++++++++++++++++ src/runtime/x86-64-arch.c | 10 ++++++---- 6 files changed, 68 insertions(+), 31 deletions(-) create mode 100644 src/runtime/unaligned.h diff --git a/src/runtime/coreparse.c b/src/runtime/coreparse.c index 4b29814d0..b7352b720 100644 --- a/src/runtime/coreparse.c +++ b/src/runtime/coreparse.c @@ -330,6 +330,7 @@ static void adjust_pointers(lispobj *where, sword_t n_words, struct heap_adjust* } #include "var-io.h" +#include "unaligned.h" static void __attribute__((unused)) adjust_code_refs(struct heap_adjust* adj, lispobj fixups, struct code* code) { @@ -343,8 +344,8 @@ adjust_code_refs(struct heap_adjust* adj, lispobj fixups, struct code* code) loc += prev_loc; prev_loc = loc; int* fixup_where = (int*)(instructions + loc); - lispobj ptr = (lispobj)(*fixup_where); - *fixup_where = (int)(ptr + calc_adjustment(adj, ptr)); + lispobj ptr = UNALIGNED_LOAD32(fixup_where); + UNALIGNED_STORE32(fixup_where, ptr + calc_adjustment(adj, ptr)); } } @@ -794,7 +795,7 @@ load_core_file(char *file, os_vm_offset_t file_offset) SHOW("found CORE_MAGIC"); #define WORD_FMTX OS_VM_SIZE_FMTX - for ( ; val != END_CORE_ENTRY_TYPE_CODE ; ptr += remaining_len) { + for ( ; ; ptr += remaining_len) { val = *ptr++; len = *ptr++; remaining_len = len - 2; /* (-2 to cancel the two ++ operations) */ @@ -804,8 +805,9 @@ load_core_file(char *file, os_vm_offset_t file_offset) switch (val) { case END_CORE_ENTRY_TYPE_CODE: - SHOW("END_CORE_ENTRY_TYPE_CODE case"); - break; + free(header); + close(fd); + return initial_function; case BUILD_ID_CORE_ENTRY_TYPE_CODE: SHOW("BUILD_ID_CORE_ENTRY_TYPE_CODE case"); @@ -877,11 +879,6 @@ load_core_file(char *file, os_vm_offset_t file_offset) lose("unknown core file entry: 0x%"WORD_FMTX"\n", val); } } - SHOW("about to free(header)"); - free(header); - close(fd); - SHOW("returning from load_core_file(..)"); - return initial_function; } #include "genesis/hash-table.h" diff --git a/src/runtime/hopscotch.c b/src/runtime/hopscotch.c index b3644a71f..4311f03e4 100644 --- a/src/runtime/hopscotch.c +++ b/src/runtime/hopscotch.c @@ -60,7 +60,7 @@ uint32_t hopscotch_hmix(uword_t key) /// Set a single bit in the hop mask for logical cell at 'index' static inline void set_hop_bit(tableptr ht, unsigned index, int bit) { - unsigned mask = 1<hops[index] |= mask; } /// Set all the bits in the hop mask for logical cell at 'index' @@ -564,7 +564,7 @@ int hopscotch_insert(tableptr ht, uword_t key, sword_t val) put_pair(physical_elt, 0, 0); // This logical bin no longer owns the index where the victim was, // but does own the index where it got moved to. - set_hop_mask(ht, logical_bin, bits ^ (1< #include @@ -605,7 +606,7 @@ static int next_varyobj_root_page(unsigned int page_index, if (varyobj_page_gens_augmented(page_index) & genmask) return page_index; else { - word ^= (1< 1 && can_wp_varyobj_page(page, keep_gen, new_gen)) - varyobj_page_touched_bits[(page - FIRST_VARYOBJ_PAGE)/32] &= ~(1<<(page & 31)); + varyobj_page_touched_bits[(page - FIRST_VARYOBJ_PAGE)/32] &= ~(1U<<(page & 31)); continue; } lispobj* page_base = (lispobj*)low_page_address(page); @@ -1130,7 +1131,7 @@ sweep_varyobj_pages(int raise) COMPUTE_NEW_MASK(mask, VARYOBJ_PAGE_GENS(page)); VARYOBJ_PAGE_GENS(page) = mask; if ( mask && wp_it ) - varyobj_page_touched_bits[(page - FIRST_VARYOBJ_PAGE)/32] &= ~(1 << (page & 31)); + varyobj_page_touched_bits[(page - FIRST_VARYOBJ_PAGE)/32] &= ~(1U << (page & 31)); } #ifdef DEBUG verify_immobile_page_protection(keep_gen, new_gen); @@ -1284,9 +1285,9 @@ void immobile_space_coreparse(uword_t fixedobj_len, uword_t varyobj_len) // Write-protect the pages occupied by the core file. // (There can be no inter-generation pointers.) if (ENABLE_PAGE_PROTECTION) { - int page; + low_page_index_t page; for (page = FIRST_VARYOBJ_PAGE ; page <= last_page ; ++page) - varyobj_page_touched_bits[(page-FIRST_VARYOBJ_PAGE)/32] &= ~(1<<(page & 31)); + varyobj_page_touched_bits[(page-FIRST_VARYOBJ_PAGE)/32] &= ~(1U<<(page & 31)); } compute_immobile_space_bound(); lispobj* code = (lispobj*)address; @@ -1407,7 +1408,7 @@ int immobile_space_handle_wp_violation(void* fault_addr) // threadsafe, so allow it anywhere. More strictness could be imparted // by tracking the max value attained by the free pointer. __sync_or_and_fetch(&varyobj_page_touched_bits[(page_index-FIRST_VARYOBJ_PAGE)/32], - 1 << (page_index & 31)); + 1U << (page_index & 31)); } else { // FIXME: a single bitmap of touched bits would make more sense, // and the _CLEARED flag doesn't achieve much if anything. @@ -1659,7 +1660,9 @@ static void adjust_fdefn_entrypoint(lispobj* where, int displacement, callee_adjust = callee_new - callee_old; } #ifdef LISP_FEATURE_X86_64 - *(int*)((char*)&fdefn->raw_addr + 1) += callee_adjust - displacement; + UNALIGNED_STORE32((char*)&fdefn->raw_addr + 1, + UNALIGNED_LOAD32((char*)&fdefn->raw_addr + 1) + + callee_adjust - displacement); #else #error "Can't adjust fdefn_raw_addr for this architecture" #endif @@ -2120,7 +2123,8 @@ void defrag_immobile_space(int* components, boolean verbose) for ( ; reloc_index < end_reloc_index ; ++reloc_index ) { unsigned char* inst_addr = (unsigned char*)(long)immobile_space_relocs[reloc_index]; gc_assert(*inst_addr == 0xE8 || *inst_addr == 0xE9); - unsigned int target_addr = (int)(long)inst_addr + 5 + *(int*)(inst_addr+1); + unsigned int target_addr = + (int)(long)inst_addr + 5 + (int)UNALIGNED_LOAD32(inst_addr+1); int target_adjust = 0; // Both this code and the jumped-to code can move. // For this component, adjust by the displacement by (old - new). @@ -2143,7 +2147,9 @@ void defrag_immobile_space(int* components, boolean verbose) char* fixup_loc = (immobile_space_p((lispobj)inst_addr) ? (char*)tempspace_addr(inst_addr - code + load_addr) : (char*)inst_addr) + 1; - *(int*)fixup_loc += target_adjust + (code - load_addr); + UNALIGNED_STORE32(fixup_loc, + UNALIGNED_LOAD32(fixup_loc) + + target_adjust + (code - load_addr)); } } #endif @@ -2262,11 +2268,11 @@ void fixup_immobile_refs(lispobj (*fixup_lispobj)(lispobj), loc += prev_loc; prev_loc = loc; int* fixup_where = (int*)(instructions + loc); - lispobj ptr = (lispobj)(*fixup_where); + lispobj ptr = (lispobj)UNALIGNED_LOAD32(fixup_where); if (is_lisp_pointer(ptr)) { lispobj fixed = fixup_lispobj(ptr); if (fixed != ptr) - *fixup_where = fixed; + UNALIGNED_STORE32(fixup_where, fixed); } else { lispobj* header_addr; if (ptr < IMMOBILE_VARYOBJ_SUBSPACE_START) { @@ -2276,8 +2282,9 @@ void fixup_immobile_refs(lispobj (*fixup_lispobj)(lispobj), gc_assert(header_addr); if (forwarding_pointer_p(header_addr)) { lispobj fpval = forwarding_pointer_value(header_addr); - *fixup_where = (int)(long)native_pointer(fpval) - + (ptr - (lispobj)header_addr); + UNALIGNED_STORE32(fixup_where, + (int)(long)native_pointer(fpval) + + (ptr - (lispobj)header_addr)); } } else if (ptr > asm_routines_end) { #ifdef LISP_FEATURE_IMMOBILE_CODE @@ -2300,7 +2307,7 @@ void fixup_immobile_refs(lispobj (*fixup_lispobj)(lispobj), // It must be the entrypoint to a static [sic] function. gc_assert(widetag_of(*tempspace_addr(native_pointer(fpval))) == SIMPLE_FUN_WIDETAG); - *fixup_where = fpval + FUN_RAW_ADDR_OFFSET; + UNALIGNED_STORE32(fixup_where, fpval + FUN_RAW_ADDR_OFFSET); } #else lose("unexpected immobile-space pointer"); diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index d55e52328..942ef061e 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -335,7 +335,7 @@ make_lispobj(void *o, int low_tag) #define MAKE_FIXNUM(n) (n << N_FIXNUM_TAG_BITS) static inline lispobj -make_fixnum(sword_t n) +make_fixnum(uword_t n) // '<<' on negatives is _technically_ undefined behavior { return MAKE_FIXNUM(n); } diff --git a/src/runtime/unaligned.h b/src/runtime/unaligned.h new file mode 100644 index 000000000..1c0128d10 --- /dev/null +++ b/src/runtime/unaligned.h @@ -0,0 +1,31 @@ +/* + * This software is part of the SBCL system. See the README file for + * more information. + * + * This software is derived from the CMU CL system, which was + * written at Carnegie Mellon University and released into the + * public domain. The software is in the public domain and is + * provided with absolutely no warranty. See the COPYING and CREDITS + * files for more information. + */ + +#ifndef _SBCL_UNALIGNED_H_ +#define _SBCL_UNALIGNED_H_ + +// For CPUs that can do unaligned memory operations, the C compiler +// is generally smart enough to not actually do a memcpy() +static inline uint32_t UNALIGNED_LOAD32(void* p) { + uint32_t val; + memcpy(&val, p, 4); + return val; +} +static inline void UNALIGNED_STORE32(void* p, uint32_t val) { + memcpy(p, &val, 4); +} +#ifdef LISP_FEATURE_64_BIT +static inline void UNALIGNED_STORE64(void* p, uint64_t val) { + memcpy(p, &val, 8); +} +#endif + +#endif /* _SBCL_UNALIGNED_H_ */ diff --git a/src/runtime/x86-64-arch.c b/src/runtime/x86-64-arch.c index 3a938085f..75437bea2 100644 --- a/src/runtime/x86-64-arch.c +++ b/src/runtime/x86-64-arch.c @@ -26,6 +26,7 @@ #include "breakpoint.h" #include "thread.h" #include "pseudo-atomic.h" +#include "unaligned.h" #include "genesis/static-symbols.h" #include "genesis/symbol.h" @@ -482,8 +483,8 @@ arch_write_linkage_table_jmp(char *reloc_addr, void *target_addr) { reloc_addr[0] = 0xFF; /* Opcode for near jump to absolute reg/mem64. */ reloc_addr[1] = 0x25; /* ModRM #b00 100 101, i.e. RIP-relative. */ - *(uint32_t*)(reloc_addr+2) = 0; /* 32-bit displacement field = 0 */ - *(uword_t *)(reloc_addr+6) = (uword_t)target_addr; + UNALIGNED_STORE32((reloc_addr+2), 0); /* 32-bit displacement field = 0 */ + UNALIGNED_STORE64((reloc_addr+6), (uword_t)target_addr); /* write a nop for good measure. */ reloc_addr[14] = 0x90; } @@ -561,8 +562,9 @@ arch_set_fp_modes(unsigned int mxcsr) lispobj fdefn_callee_lispobj(struct fdefn* fdefn) { extern unsigned asm_routines_end; if (((lispobj)fdefn->raw_addr & 0xFE) == 0xE8) { // looks good - unsigned int raw_fun = (int)(long)&fdefn->raw_addr + 5 // length of "JMP rel32" - + *(int*)((char*)&fdefn->raw_addr + 1); + int32_t offs = UNALIGNED_LOAD32((char*)&fdefn->raw_addr + 1); + unsigned int raw_fun = + (int)(long)&fdefn->raw_addr + 5 + offs; // 5 = length of "JMP rel32" switch (((unsigned char*)&fdefn->raw_addr)[5]) { case 0x00: // no closure/fin trampoline // If the target is an assembly routine, there is no simple-fun -- 2.11.4.GIT