From 459fca581ce9f1215a96885852b912558cdc9c63 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Sat, 25 Nov 2017 20:24:00 +0000 Subject: [PATCH] Replace the store_release() internal interface, which was excessively unsafe. The new store_newblock() includes the required safety checck, plus the alocate and data-copy operations. --- doc/doc-txt/ChangeLog | 8 +++++ src/src/receive.c | 8 +---- src/src/store.c | 68 +++++++++++++++++++++++++++++++------------ src/src/store.h | 18 +++++++----- src/src/string.c | 8 +---- 5 files changed, 69 insertions(+), 41 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 0ea49a280..0dd8ca60f 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -5,6 +5,14 @@ affect Exim's operation, with an unchanged configuration file. For new options, and new features, see the NewStuff file next to this ChangeLog. +Exim version 4.next +----------------- + +JH/01 Replace the store_release() internal interface with store_newblock(), + which internalises the check required to safely use the old one, plus + the allocate and data copy operations duplicated in both (!) of the + extant use locations. + Exim version 4.90 ----------------- diff --git a/src/src/receive.c b/src/src/receive.c index d9b500102..93c091c80 100644 --- a/src/src/receive.c +++ b/src/src/receive.c @@ -1819,13 +1819,7 @@ for (;;) /* header_size += 256; */ header_size *= 2; if (!store_extend(next->text, oldsize, header_size)) - { - BOOL release_ok = store_last_get[store_pool] == next->text; - uschar *newtext = store_get(header_size); - memcpy(newtext, next->text, ptr); - if (release_ok) store_release(next->text); - next->text = newtext; - } + next->text = store_newblock(next->text, header_size, ptr); } /* Cope with receiving a binary zero. There is dispute about whether diff --git a/src/src/store.c b/src/src/store.c index c7cf33c9a..8f5da3a5a 100644 --- a/src/src/store.c +++ b/src/src/store.c @@ -428,14 +428,8 @@ DEBUG(D_memory) * Release store * ************************************************/ -/* This function is specifically provided for use when reading very -long strings, e.g. header lines. When the string gets longer than a -complete block, it gets copied to a new block. It is helpful to free -the old block iff the previous copy of the string is at its start, -and therefore the only thing in it. Otherwise, for very long strings, -dead store can pile up somewhat disastrously. This function checks that -the pointer it is given is the first thing in a block, and if so, -releases that block. +/* This function checks that the pointer it is given is the first thing in a +block, and if so, releases that block. Arguments: block block of store to consider @@ -445,17 +439,17 @@ Arguments: Returns: nothing */ -void -store_release_3(void *block, const char *filename, int linenumber) +static void +store_release_3(void * block, const char * filename, int linenumber) { -storeblock *b; +storeblock * b; /* It will never be the first block, so no need to check that. */ -for (b = chainbase[store_pool]; b != NULL; b = b->next) +for (b = chainbase[store_pool]; b; b = b->next) { - storeblock *bb = b->next; - if (bb != NULL && CS block == CS bb + ALIGNED_SIZEOF_STOREBLOCK) + storeblock * bb = b->next; + if (bb && CS block == CS bb + ALIGNED_SIZEOF_STOREBLOCK) { b->next = bb->next; pool_malloc -= bb->length + ALIGNED_SIZEOF_STOREBLOCK; @@ -463,21 +457,20 @@ for (b = chainbase[store_pool]; b != NULL; b = b->next) /* Cut out the debugging stuff for utilities, but stop picky compilers from giving warnings. */ - #ifdef COMPILE_UTILITY +#ifdef COMPILE_UTILITY filename = filename; linenumber = linenumber; - #else +#else DEBUG(D_memory) - { if (running_in_test_harness) debug_printf("-Release %d\n", pool_malloc); else debug_printf("-Release %6p %-20s %4d %d\n", (void *)bb, filename, linenumber, pool_malloc); - } + if (running_in_test_harness) memset(bb, 0xF0, bb->length+ALIGNED_SIZEOF_STOREBLOCK); - #endif /* COMPILE_UTILITY */ +#endif /* COMPILE_UTILITY */ free(bb); return; @@ -486,6 +479,43 @@ for (b = chainbase[store_pool]; b != NULL; b = b->next) } +/************************************************ +* Move store * +************************************************/ + +/* Allocate a new block big enough to expend to the given size and +copy the current data into it. Free the old one if possible. + +This function is specifically provided for use when reading very +long strings, e.g. header lines. When the string gets longer than a +complete block, it gets copied to a new block. It is helpful to free +the old block iff the previous copy of the string is at its start, +and therefore the only thing in it. Otherwise, for very long strings, +dead store can pile up somewhat disastrously. This function checks that +the pointer it is given is the first thing in a block, and that nothing +has been allocated since. If so, releases that block. + +Arguments: + block + newsize + len + +Returns: new location of data +*/ + +void * +store_newblock_3(void * block, int newsize, int len, + const char * filename, int linenumber) +{ +BOOL release_ok = store_last_get[store_pool] == block; +uschar * newtext = store_get(newsize); + +memcpy(newtext, block, len); +if (release_ok) store_release_3(block, filename, linenumber); +return (void *)newtext; +} + + /************************************************* diff --git a/src/src/store.h b/src/src/store.h index 7c860f154..cb0a3cae9 100644 --- a/src/src/store.h +++ b/src/src/store.h @@ -34,19 +34,21 @@ tracing information for debugging. */ #define store_get(size) store_get_3(size, __FILE__, __LINE__) #define store_get_perm(size) store_get_perm_3(size, __FILE__, __LINE__) #define store_malloc(size) store_malloc_3(size, __FILE__, __LINE__) -#define store_release(addr) store_release_3(addr, __FILE__, __LINE__) +#define store_newblock(addr,newsize,datalen) \ + store_newblock_3(addr, newsize, datalen, __FILE__, __LINE__) #define store_reset(addr) store_reset_3(addr, __FILE__, __LINE__) /* The real functions */ -extern BOOL store_extend_3(void *, int, int, const char *, int); /* The */ -extern void store_free_3(void *, const char *, int); /* value of the */ -extern void *store_get_3(int, const char *, int); /* 2nd arg is */ -extern void *store_get_perm_3(int, const char *, int); /* __FILE__ in */ -extern void *store_malloc_3(int, const char *, int); /* every call, */ -extern void store_release_3(void *, const char *, int); /* so give its */ -extern void store_reset_3(void *, const char *, int); /* correct type */ +/* The value of the 2nd arg is __FILE__ in every call, so give its correct type */ +extern BOOL store_extend_3(void *, int, int, const char *, int); +extern void store_free_3(void *, const char *, int); +extern void *store_get_3(int, const char *, int); +extern void *store_get_perm_3(int, const char *, int); +extern void *store_malloc_3(int, const char *, int); +extern void *store_newblock_3(void *, int, int, const char *, int); +extern void store_reset_3(void *, const char *, int); #endif /* STORE_H */ diff --git a/src/src/string.c b/src/src/string.c index 2e919e6d9..81aacb94b 100644 --- a/src/src/string.c +++ b/src/src/string.c @@ -1102,13 +1102,7 @@ was the last item on the dynamic memory stack. This is the case if it matches store_last_get. */ if (!store_extend(g->s, oldsize, g->size)) - { - BOOL release_ok = store_last_get[store_pool] == g->s; - uschar *newstring = store_get(g->size); - memcpy(newstring, g->s, p); - if (release_ok) store_release(g->s); - g->s = newstring; - } + g->s = store_newblock(g->s, g->size, p); } -- 2.30.2