From 632b5f146cfff1f2b1188a0db0f1db6bc6d48c54 Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Wed, 4 Jul 2018 17:36:44 +0100 Subject: [PATCH] Extend MEMORY_CANARY support to put guards on every Bag --- src/gasman.c | 77 ++++++++++++++++++++++++++++++++++++++++------------ src/gasman.h | 5 ++++ 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/gasman.c b/src/gasman.c index f68298c8eea..463cc8ecb1f 100644 --- a/src/gasman.c +++ b/src/gasman.c @@ -468,24 +468,55 @@ static inline UInt IS_BAG_BODY(void * ptr) #include #include -static Int canary_size(void) + +// tell valgrind that the masterpointer, bag contents and bag header of Bag +// should all be accessible +static void CANARY_ALLOW_ACCESS_BAG(Bag bag) { - Int bufsize = (Int)EndBags - (Int)AllocBags; - return bufsize < 4096 ? bufsize : 4096; + VALGRIND_MAKE_MEM_DEFINED(bag, sizeof(Bag)); + char * ptr = (char *)PTR_BAG(bag); + Int bagLength = SIZE_BAG(bag); + VALGRIND_MAKE_MEM_DEFINED(ptr, bagLength); + + BagHeader * header = BAG_HEADER(bag); + VALGRIND_MAKE_MEM_DEFINED( + header, sizeof(*header) - sizeof(header->memory_canary_padding)); } -static void ADD_CANARY(void) +// Reverse CANARY_ALL_ACCESS_BAG, making the masterpointer, bag contents and +// bag header all inaccessible +static void CANARY_FORBID_ACCESS_BAG(Bag bag) { - VALGRIND_MAKE_MEM_NOACCESS(AllocBags, canary_size()); + VALGRIND_MAKE_MEM_NOACCESS(bag, sizeof(Bag)); + char * ptr = (char *)PTR_BAG(bag); + Int bagLength = SIZE_BAG(bag); + VALGRIND_MAKE_MEM_NOACCESS(ptr, bagLength); + + BagHeader * header = BAG_HEADER(bag); + VALGRIND_MAKE_MEM_NOACCESS( + header, sizeof(*header) - sizeof(header->memory_canary_padding)); } -static void CLEAR_CANARY(void) +// Mark all bags as accessible +static void CANARY_ALLOW_ACCESS_ALL_BAGS(void) { - VALGRIND_MAKE_MEM_DEFINED(AllocBags, canary_size()); + CallbackForAllBags(CANARY_ALLOW_ACCESS_BAG); } + +// Mark all bags as inaccessible +static void CANARY_FORBID_ACCESS_ALL_BAGS(void) +{ + VALGRIND_MAKE_MEM_NOACCESS(MptrBags, (EndBags - MptrBags) * sizeof(Bag)); +} + +// Temporarily disable valgrind checking. This is used while creating bags or +// adjusting any internal GASMAN structures #define CANARY_DISABLE_VALGRIND() VALGRIND_DISABLE_ERROR_REPORTING + +// Renable valgrind checking. #define CANARY_ENABLE_VALGRIND() VALGRIND_ENABLE_ERROR_REPORTING +// CHANGED_BAG must be here to disable/enable valgrind void CHANGED_BAG(Bag bag) { CANARY_DISABLE_VALGRIND(); @@ -496,10 +527,12 @@ void CHANGED_BAG(Bag bag) CANARY_ENABLE_VALGRIND(); } #else -#define ADD_CANARY() -#define CLEAR_CANARY() #define CANARY_DISABLE_VALGRIND() #define CANARY_ENABLE_VALGRIND() +#define CANARY_ALLOW_ACCESS_BAG(b) +#define CANARY_FORBID_ACCESS_BAG(b) +#define CANARY_ALLOW_ACCESS_ALL_BAGS() +#define CANARY_FORBID_ACCESS_ALL_BAGS() #endif /**************************************************************************** @@ -1212,6 +1245,7 @@ void InitBags ( ChangedBags = 0; GAP_ASSERT(SanityCheckGasmanPointers()); + CANARY_FORBID_ACCESS_ALL_BAGS(); } @@ -1264,6 +1298,8 @@ Bag NewBag ( CollectBags(0,0); #endif + CANARY_DISABLE_VALGRIND(); + /* check that a masterpointer and enough storage are available */ if ( (FreeMptrBags == 0 || SizeAllocationArea < WORDS_BAG(sizeof(BagHeader)+size)) && CollectBags( size, 0 ) == 0 ) @@ -1286,11 +1322,10 @@ Bag NewBag ( /* get the identifier of the bag and set 'FreeMptrBags' to the next */ bag = FreeMptrBags; FreeMptrBags = *(Bag*)bag; - CLEAR_CANARY(); + /* allocate the storage for the bag */ BagHeader * header = (BagHeader *)AllocBags; AllocBags = DATA(header) + WORDS_BAG(size); - ADD_CANARY(); // enter bag header header->type = type; @@ -1306,8 +1341,12 @@ Bag NewBag ( /* set the masterpointer */ SET_PTR_BAG(bag, DATA(header)); + CANARY_ALLOW_ACCESS_BAG(bag); + GAP_ASSERT(SanityCheckGasmanPointers()); + CANARY_ENABLE_VALGRIND(); + /* return the identifier of the new bag */ return bag; } @@ -1439,6 +1478,10 @@ UInt ResizeBag ( CollectBags(0,0); #endif + CANARY_DISABLE_VALGRIND(); + + CANARY_FORBID_ACCESS_BAG(bag); + BagHeader * header = BAG_HEADER(bag); UInt type = header->type; UInt flags = header->flags; @@ -1488,7 +1531,6 @@ UInt ResizeBag ( // if the last bag is enlarged ... else if (CONST_PTR_BAG(bag) + WORDS_BAG(old_size) == AllocBags) { - CLEAR_CANARY(); // check that enough storage for the new bag is available if (SpaceBetweenPointers(EndBags, CONST_PTR_BAG(bag)) < WORDS_BAG(new_size) && CollectBags( new_size-old_size, 0 ) == 0 ) { @@ -1509,8 +1551,6 @@ UInt ResizeBag ( #endif SizeAllBags += new_size - old_size; - ADD_CANARY(); - header->size = new_size; #if SIZEOF_VOID_P == 4 GAP_ASSERT(header->reserved == 0); @@ -1525,7 +1565,6 @@ UInt ResizeBag ( && CollectBags( new_size, 0 ) == 0 ) { Panic("Cannot extend the workspace any more!!!!!!"); } - CLEAR_CANARY(); // update header pointer in case bag moved header = BAG_HEADER(bag); @@ -1542,7 +1581,6 @@ UInt ResizeBag ( /* allocate the storage for the bag */ BagHeader * newHeader = (BagHeader *)AllocBags; AllocBags = DATA(newHeader) + WORDS_BAG(new_size); - ADD_CANARY(); newHeader->type = type; newHeader->flags = flags; @@ -1586,6 +1624,8 @@ UInt ResizeBag ( } GAP_ASSERT(SanityCheckGasmanPointers()); + CANARY_ALLOW_ACCESS_BAG(bag); + CANARY_ENABLE_VALGRIND(); /* return success */ return 1; } @@ -1873,7 +1913,7 @@ UInt CollectBags ( GAP_ASSERT(SanityCheckGasmanPointers()); CANARY_DISABLE_VALGRIND(); - CLEAR_CANARY(); + CANARY_FORBID_ACCESS_ALL_BAGS(); #ifdef DEBUG_MASTERPOINTERS CheckMasterPointers(); #endif @@ -2056,7 +2096,7 @@ UInt CollectBags ( /* free the identifier */ *(Bag*)(header->link) = FreeMptrBags; FreeMptrBags = header->link; - + /* advance src */ src = DATA(header) + WORDS_BAG( header->size ) ; @@ -2325,6 +2365,7 @@ UInt CollectBags ( /* Possibly advise the operating system about unused pages: */ SyMAdviseFree(); + CANARY_ALLOW_ACCESS_ALL_BAGS(); CANARY_ENABLE_VALGRIND(); GAP_ASSERT(SanityCheckGasmanPointers()); diff --git a/src/gasman.h b/src/gasman.h index e362596e913..c9c1501cf18 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -90,6 +90,11 @@ typedef struct { #ifdef USE_GASMAN Bag link; #endif +#if defined(MEMORY_CANARY) + // The following variable is marked as not readable or writable + // in valgrind, to check for code reading before the start of a Bag. + uint64_t memory_canary_padding[8]; +#endif } BagHeader;