Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete the page pool allocator #280

Merged
merged 4 commits into from
Feb 10, 2022
Merged

Delete the page pool allocator #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8762669
Delete the page pool allocator
sadderchris Dec 20, 2021
fdbf6d4
fix: Use after free in tests
sadderchris Feb 3, 2022
3be6a9e
fix: fix heap use-after-free in IonBinaryDecimal.RoundtripPreservesFu…
sadderchris Feb 9, 2022
18d9ef9
fix: Fix heap use-after-free in ion extractor test
sadderchris Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 0 additions & 42 deletions ionc/ion_alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,48 +131,6 @@ typedef struct _ion_allocation_chain DBG_ION_ALLOCATION_CHAIN;
#endif


//
// structures and functions for the ion allocation page pool
// this is a list of free pages which are used by the various
// pools created by the Ion routines on behalf of the various
// objects, such as the reader, writer, or catalog.
//
// THIS IS NOT THREAD SAFE - (which could be corrected)
//

typedef struct _ion_alloc_page ION_ALLOC_PAGE;
typedef struct _ion_alloc_page_list ION_ALLOC_PAGE_LIST;

struct _ion_alloc_page
{
ION_ALLOC_PAGE *next;
};

struct _ion_alloc_page_list
{
SIZE page_size;
int page_count;
int free_page_limit;
ION_ALLOC_PAGE *head;
};

#define ION_ALLOC_PAGE_POOL_NO_LIMIT (-1)
#define ION_ALLOC_PAGE_POOL_DEFAULT_LIMIT (16)
#define ION_ALLOC_PAGE_MIN_SIZE (ALIGN_SIZE(sizeof(ION_ALLOCATION_CHAIN)))
#define ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE (DEFAULT_BLOCK_SIZE)
#define ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE (-1)

GLOBAL THREAD_LOCAL_STORAGE ION_ALLOC_PAGE_LIST g_ion_alloc_page_list
#ifdef INIT_STATICS
= { ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE, 0, ION_ALLOC_PAGE_POOL_DEFAULT_LIMIT, NULL }
#endif
;

ION_API_EXPORT void ion_initialize_page_pool (SIZE page_size, int free_page_limit);
ION_API_EXPORT void ion_release_page_pool (void);

ION_ALLOC_PAGE *_ion_alloc_page (void);
void _ion_release_page (ION_ALLOC_PAGE *page);

void *_ion_alloc_owner (SIZE len);
void *_ion_alloc_with_owner(hOWNER owner, SIZE length);
Expand Down
105 changes: 8 additions & 97 deletions ionc/ion_allocation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ void *_ion_alloc_with_owner_helper(ION_ALLOCATION_CHAIN *powner, SIZE request_le
pblock = _ion_alloc_block(length);
if (!pblock) return NULL;

if (pblock->size > g_ion_alloc_page_list.page_size && powner->head != NULL) {
if (pblock->size > DEFAULT_BLOCK_SIZE && powner->head != NULL) {
// this is an oversized block, so don't put it
// at the front since it will be full and we'll
// have wasted the freespace in the current
Expand Down Expand Up @@ -178,121 +178,32 @@ ION_ALLOCATION_CHAIN *_ion_alloc_block(SIZE min_needed)
ION_ALLOCATION_CHAIN *new_block;
SIZE alloc_size = min_needed + ALIGN_SIZE(sizeof(ION_ALLOCATION_CHAIN)); // subtract out the block[1]

if (alloc_size > g_ion_alloc_page_list.page_size) {
// it's an oversize block - we'll ask the system for this one
new_block = (ION_ALLOCATION_CHAIN *)ion_xalloc(alloc_size);
new_block->size = alloc_size;
}
else {
// it's a normal size block - go out to the block pool for it
new_block = (ION_ALLOCATION_CHAIN *)_ion_alloc_page();
if (new_block) {
new_block->size = g_ion_alloc_page_list.page_size;
}
}
if (alloc_size < DEFAULT_BLOCK_SIZE) alloc_size = DEFAULT_BLOCK_SIZE;

new_block = (ION_ALLOCATION_CHAIN *)ion_xalloc(alloc_size);

// see if we suceeded
if (!new_block) return NULL;


new_block->size = alloc_size;
new_block->next = NULL;
new_block->head = NULL;

new_block->position = ION_ALLOC_BLOCK_TO_USER_PTR(new_block);
new_block->limit = ((BYTE*)new_block) + new_block->size;

assert(new_block->position == ((BYTE *)(&new_block->limit) + ALIGN_SIZE(sizeof(new_block->limit))));
assert(new_block->position == ((BYTE *)ALIGN_PTR((BYTE *)(&new_block->limit) + ALIGN_SIZE(sizeof(new_block->limit)))));

return new_block;
}

void _ion_free_block(ION_ALLOCATION_CHAIN *pblock)
{
if (!pblock) return;
if (pblock->size > g_ion_alloc_page_list.page_size) {
ion_xfree(pblock);
}
else {
_ion_release_page((ION_ALLOC_PAGE *)pblock);
}
return;
}

void ion_initialize_page_pool(SIZE page_size, int free_page_limit)
{
// TODO This is always true, causing this function to do nothing, because g_ion_alloc_page_list.page_size
// is statically initialized to ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE. The original intent of this check
// (when it was != ) was to attempt to determine whether the page pool had already been initialized,
// however the check was insufficient. The problem of how to safely allow users to configure the page
// pool exactly once needs to be solved. See https://github.com/amzn/ion-c/issues/242 .
if (g_ion_alloc_page_list.page_size == ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE)
{
return;
}

// we need a min size to hold the pointers we use to maintain the page list
if (page_size < ION_ALLOC_PAGE_MIN_SIZE)
{
page_size = ION_ALLOC_PAGE_MIN_SIZE;
}
g_ion_alloc_page_list.page_size = page_size;
g_ion_alloc_page_list.free_page_limit = free_page_limit;

// TODO: should we pre-allocate the pages?

return;
}

void ion_release_page_pool(void)
{
ION_ALLOC_PAGE *page;

while ((page = g_ion_alloc_page_list.head) != NULL) {
g_ion_alloc_page_list.head = page->next;
ion_xfree(page);
g_ion_alloc_page_list.page_count--;
}
ASSERT(g_ion_alloc_page_list.page_count == 0);

// here we mark the page size to indicate the pool is inactive
g_ion_alloc_page_list.page_size = ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE;

return;
}

ION_ALLOC_PAGE *_ion_alloc_page(void)
{
ION_ALLOC_PAGE *page;

if ((page = g_ion_alloc_page_list.head) != NULL) {
g_ion_alloc_page_list.head = page->next;
g_ion_alloc_page_list.page_count--;
}
else {
ASSERT(g_ion_alloc_page_list.page_size != ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE);
page = ion_xalloc(g_ion_alloc_page_list.page_size);
}
return page;
}

void _ion_release_page(ION_ALLOC_PAGE *page)
{
if (page == NULL) return;

if (g_ion_alloc_page_list.free_page_limit != ION_ALLOC_PAGE_POOL_NO_LIMIT
&& g_ion_alloc_page_list.page_count >= g_ion_alloc_page_list.free_page_limit
) {
ion_xfree(page);
}
else {
page->next = g_ion_alloc_page_list.head;
g_ion_alloc_page_list.head = page;
g_ion_alloc_page_list.page_count++;
}
ion_xfree(pblock);
return;
}



#ifdef MEM_DEBUG

long malloc_inuse = 0;
Expand Down
2 changes: 1 addition & 1 deletion test/test_ion_binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,13 +265,13 @@ TEST(IonBinarySymbol, ReaderReadsSymbolValueZeroAsSID) {
ION_ASSERT_OK(ion_reader_next(reader, &actual_type));
ASSERT_EQ(tid_SYMBOL, actual_type);
ION_ASSERT_OK(ion_test_reader_read_symbol_sid(reader, &actual));
ION_ASSERT_OK(ion_reader_close(reader));

ASSERT_EQ(0, actual);

ION_ASSERT_OK(ion_reader_get_symbol_table(reader, &symbol_table));
ION_ASSERT_OK(ion_symbol_table_find_by_sid(symbol_table, 0, &symbol_value));
ASSERT_TRUE(ION_STRING_IS_NULL(symbol_value));
ION_ASSERT_OK(ion_reader_close(reader));
}

TEST(IonBinarySymbol, WriterWritesSymbolValueIVM) {
Expand Down
34 changes: 18 additions & 16 deletions test/test_ion_decimal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@

/* Reading/writing test utilities */

#define ION_DECIMAL_READER_NEXT \
ION_ASSERT_OK(ion_reader_next(reader, &type)); \
#define ION_DECIMAL_READER_NEXT(reader) \
ION_ASSERT_OK(ion_reader_next((reader), &type)); \
ASSERT_EQ(tid_DECIMAL, type);

#define ION_DECIMAL_WRITER_INIT(is_binary) \
Expand Down Expand Up @@ -102,7 +102,7 @@
*/
#define ION_DECIMAL_EXPECT_OVERFLOW(func, max_digits) \
ION_DECIMAL_READER_INIT_CUSTOM_OPTIONS(max_digits); \
ION_DECIMAL_READER_NEXT; \
ION_DECIMAL_READER_NEXT(reader); \
ASSERT_EQ(IERR_NUMERIC_OVERFLOW, func); \
ION_ASSERT_OK(ion_reader_close(reader));

Expand All @@ -115,13 +115,13 @@
ION_DECIMAL_READER_INIT; \
ION_DECIMAL_WRITER_INIT(TRUE); \
\
ION_DECIMAL_READER_NEXT; \
ION_DECIMAL_READER_NEXT(reader); \
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal)); \
ASSERT_TRUE(ION_DECIMAL_IS_NUMBER((&ion_decimal))); \
ASSERT_EQ(decimal_digits, ion_decimal.value.num_value->digits); \
\
ION_ASSERT_OK(ion_writer_write_ion_decimal(writer, &ion_decimal)); \
ION_DECIMAL_CLOSE_READER_WRITER;
ION_ASSERT_OK(ion_test_writer_get_bytes(writer, ion_stream, &result, &result_len));

#define ION_DECIMAL_BINARY_READER_EXPECT_OVERFLOW(func, decimal_digits) \
ION_DECIMAL_TEXT_TO_BINARY(decimal_digits); \
Expand All @@ -140,7 +140,7 @@ TEST(IonTextDecimal, RoundtripPreservesFullFidelityDecNumber) {
ION_DECIMAL_READER_INIT;
ION_DECIMAL_WRITER_INIT(FALSE);

ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal));
ION_ASSERT_OK(ion_writer_write_ion_decimal(writer, &ion_decimal));

Expand All @@ -151,16 +151,18 @@ TEST(IonTextDecimal, RoundtripPreservesFullFidelityDecNumber) {
TEST(IonBinaryDecimal, RoundtripPreservesFullFidelityDecNumber) {
const char *text_decimal = "1.1999999999999999555910790149937383830547332763671875";
ION_DECIMAL ion_decimal_after;
hREADER test_reader;

ION_DECIMAL_TEXT_TO_BINARY(53);

ION_ASSERT_OK(ion_test_new_reader(result, (SIZE)result_len, &reader));
ION_DECIMAL_READER_NEXT;
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal_after));
ION_ASSERT_OK(ion_reader_close(reader));
ION_ASSERT_OK(ion_test_new_reader(result, (SIZE)result_len, &test_reader));
ION_DECIMAL_READER_NEXT(test_reader);
ION_ASSERT_OK(ion_reader_read_ion_decimal(test_reader, &ion_decimal_after));

ASSERT_TRUE(ion_equals_decimal(&ion_decimal, &ion_decimal_after));

ION_ASSERT_OK(ion_reader_close(test_reader));
ION_ASSERT_OK(ion_reader_close(reader));
free(result);
ION_DECIMAL_FREE_2(&ion_decimal, &ion_decimal_after);
}
Expand Down Expand Up @@ -217,9 +219,9 @@ TEST(IonTextDecimal, ReaderAlwaysPreservesUpTo34Digits) {
ION_DECIMAL_READER_INIT_CUSTOM_OPTIONS(3);
ION_DECIMAL_WRITER_INIT(FALSE);

ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal));
ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_decimal(reader, &quad));

ION_ASSERT_OK(ion_writer_write_ion_decimal(writer, &ion_decimal));
Expand All @@ -242,21 +244,21 @@ TEST(IonBinaryDecimal, ReaderAlwaysPreservesUpTo34Digits) {
ION_DECIMAL_READER_INIT;
ION_DECIMAL_WRITER_INIT(TRUE);

ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal_before));
ASSERT_EQ(ION_DECIMAL_TYPE_QUAD, ion_decimal_before.type);
ASSERT_EQ(4, decQuadDigits(&ion_decimal_before.value.quad_value));
ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_decimal(reader, &quad_before));

ION_ASSERT_OK(ion_writer_write_ion_decimal(writer, &ion_decimal_before));
ION_ASSERT_OK(ion_writer_write_decimal(writer, &quad_before));
ION_DECIMAL_CLOSE_READER_WRITER;

ION_DECIMAL_READER_INIT_CUSTOM_OPTIONS(3);
ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_ion_decimal(reader, &ion_decimal_after));
ION_DECIMAL_READER_NEXT;
ION_DECIMAL_READER_NEXT(reader);
ION_ASSERT_OK(ion_reader_read_decimal(reader, &quad_after));

ION_ASSERT_OK(ion_decimal_equals(&ion_decimal_before, &ion_decimal_after, &((ION_READER *)reader)->_deccontext, &decimal_equals));
Expand Down
5 changes: 4 additions & 1 deletion test/test_ion_extractor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,10 +476,13 @@ TEST(IonExtractorSucceedsWhen, MultiplePathsCreatedUpFrontMatch) {
assertion_contexts[path2->_path_id].path = path2;
assertion_contexts[path3->_path_id].path = path3;

ION_EXTRACTOR_TEST_MATCH;
ION_ASSERT_OK(ion_test_new_text_reader(ion_text, &reader)); \
ION_ASSERT_OK(ion_extractor_match(extractor, reader)); \
ION_EXTRACTOR_TEST_ASSERT_MATCHED(path->_path_id, 1);
ION_EXTRACTOR_TEST_ASSERT_MATCHED(path2->_path_id, 1);
ION_EXTRACTOR_TEST_ASSERT_MATCHED(path3->_path_id, 1);
ION_ASSERT_OK(ion_extractor_close(extractor)); \
ION_ASSERT_OK(ion_reader_close(reader));

}

Expand Down
4 changes: 0 additions & 4 deletions tools/ionizer/ionizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ int main(int argc, char **argv)
g_writer_options.output_as_binary = g_ionizer_write_binary;
g_writer_options.escape_all_non_ascii = g_ionizer_ascii_only;

if (g_ionizer_pool_page_size > 0) {
ion_initialize_page_pool(g_ionizer_pool_page_size, 10);
}

// set up our debug options
if (g_ionizer_flush_each) g_writer_options.flush_every_value = TRUE;
if (g_ionizer_dump_args) ionizer_dump_arg_globals();
Expand Down
1 change: 0 additions & 1 deletion tools/ionizer/ionizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ IZ_GLOBAL BOOL g_ionizer_debug IZ_INITTO(FALSE);
IZ_GLOBAL BOOL g_ionizer_dump_args IZ_INITTO(FALSE);

IZ_GLOBAL int g_ionizer_symtab_version IZ_INITTO(0);
IZ_GLOBAL int g_ionizer_pool_page_size IZ_INITTO(-1);

// IZ_GLOBAL char g_ionizer_symtab_name[MAX_FILE_NAME_LEN + 1] IZ_INITTO({0});
// IZ_GLOBAL char g_ionizer_catalog[MAX_FILE_NAME_LEN + 1] IZ_INITTO({0});
Expand Down
8 changes: 0 additions & 8 deletions tools/ionizer/ionizer_args.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ BOOL set_name(OC *pcur);
BOOL set_symbol_table(OC *pcur);
BOOL set_catalog_name(OC *pcur);
BOOL set_version(OC *pcur);
BOOL set_pagesize(OC *pcur);
BOOL set_write_binary(OC *pcur);
BOOL set_write_symtab(OC *pcur);
BOOL set_ugly(OC *pcur);
Expand All @@ -54,7 +53,6 @@ OPT_DEF inionizer_options[] = {
{ ot_int, 't', "trace", TRUE, FALSE, set_trace, "turns on trace options 1:args,2:parse state,3:parse fns,4:tokens,5:calls,6:time" },
{ ot_string, 'n', "name", FALSE, FALSE, set_name, "sets the output symbol table name" },
{ ot_string, 'o', "output", FALSE, FALSE, set_output, "sets the output format: ugly, binary, pretty, counts, none(scan only), types(counts)" },
{ ot_int, 'p', "pagesize", TRUE, FALSE, set_pagesize, "set the page size of the memory pool" },
{ ot_string, 's', "symbol_table", FALSE, FALSE, set_symbol_table, "symbol table file for the writer (uses newest version)" },
{ ot_none, 'u', NULL, FALSE, FALSE, set_ugly, "sets the output format to ugly" },
{ ot_int, 'v', "version", FALSE, FALSE, set_version, "set the output symbol tables version" },
Expand Down Expand Up @@ -141,12 +139,6 @@ BOOL set_version(OC *pcur) {
g_ionizer_symtab_version = atoi(val);
return TRUE;
}
BOOL set_pagesize(OC *pcur) {
char * val = opt_get_arg(pcur);
if (!val) return FALSE;
g_ionizer_pool_page_size = atoi(val);
return TRUE;
}

BOOL set_write_binary(OC *pcur) {
g_ionizer_write_binary = TRUE;
Expand Down