laria/apfl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix apfl_join_strings accessing invalid addresses

Browse source 
Previously we were getting a pointer into the value stack for the parts
list. However, that pointer would not necessarily be a valid pointer later
any more, as we're also manipulating the value stack.

This introduces the higher level API apfl_set_list_member_by_index to
modify a list and also adds an option for our test suite to run the tests
with valgrind --tool=memcheck. This should catch such errors in the future.
This commit is contained in:
Laria 2022-11-29 23:13:10 +01:00
parent e4f2053ca6
commit 57cd32dfa5
3 changed files with 61 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
src/CMakeLists.txt
View file

@ -1,6 +1,8 @@
set(CMAKE_C_STANDARD 11)
set(CMAKE_C_EXTENSIONS OFF)
option(TEST_WITH_VALGRIND_MEMCHECK "Also run tests with valgrind / memcheck" ON)
add_library(apfl
alloc.c
bytecode.c
@ -39,6 +41,10 @@ function(unittest name moresources)
add_executable(${name} test.h ${name}.c ${moresources})
target_link_libraries(${name} PUBLIC apfl)
add_test(NAME ${name} COMMAND ${name})
if(${TEST_WITH_VALGRIND_MEMCHECK})
add_test(NAME "valgrind_memcheck__${name}" COMMAND valgrind --tool=memcheck --leak-check=full -q --error-exitcode=99 $<TARGET_FILE:${name}>)
endif()
endfunction()
unittest(tokenizer_test "")
@ -49,6 +55,9 @@ unittest(strings_test "")
function(functionaltest name)
add_test(NAME "functionaltest_${name}" COMMAND functional-test-runner ${CMAKE_SOURCE_DIR}/src/functional-tests/${name}.at)
if(${TEST_WITH_VALGRIND_MEMCHECK})
add_test(NAME "valgrind_memcheck__functionaltest_${name}" COMMAND valgrind --tool=memcheck --leak-check=full -q --error-exitcode=99 $<TARGET_FILE:functional-test-runner> ${CMAKE_SOURCE_DIR}/src/functional-tests/${name}.at)
endif()
endfunction()
functionaltest("hello-world")

2
src/apfl.h
View file

@ -692,6 +692,8 @@ void apfl_get_member(apfl_ctx, apfl_stackidx container, apfl_stackidx k);
bool apfl_get_member_if_exists(apfl_ctx, apfl_stackidx container, apfl_stackidx k);
// Get a value from a list and push it on the stack. The list will stay on the stack.
void apfl_get_list_member_by_index(apfl_ctx, apfl_stackidx list, size_t index_in_list);
// Pop a value from the stack and replace the index_in_list-th entry in list with it. The index must already exist.
void apfl_set_list_member_by_index(apfl_ctx, apfl_stackidx list, size_t index_in_list, apfl_stackidx value);
// Get the length of a string/list/dict. The value stays on the stack,
size_t apfl_len(apfl_ctx, apfl_stackidx);
// Get a string view into a APFL_VALUE_STRING value. The value stays on the stack.

82
src/context.c
View file

@ -1176,8 +1176,42 @@ apfl_get_list_member_by_index(
}
*value = apfl_value_set_cow_flag(list.list->items[index]);
}
return;
void
apfl_set_list_member_by_index(
apfl_ctx ctx,
apfl_stackidx list_index,
size_t index_in_list,
apfl_stackidx value_index
) {
struct apfl_value *list = stack_get_pointer(ctx, list_index);
if (list == NULL) {
apfl_raise_invalid_stackidx(ctx);
}
if (list->type != VALUE_LIST) {
apfl_raise_const_error(ctx, APFL_RESULT_ERR, apfl_messages.not_a_list);
}
if (index_in_list >= list->list->len) {
apfl_raise_const_error(ctx, APFL_RESULT_ERR, apfl_messages.key_doesnt_exist);
}
struct apfl_value value = apfl_stack_must_get(ctx, value_index);
if (!apfl_list_splice(
&ctx->gc,
&list->list,
index_in_list,
1,
&value,
1
)) {
apfl_raise_alloc_error(ctx);
}
apfl_drop(ctx, value_index);
}
size_t
@ -1283,45 +1317,28 @@ apfl_join_strings(apfl_ctx ctx, apfl_stackidx glue, apfl_stackidx parts)
parts = -2;
glue = -1;
struct apfl_value *parts_val = stack_get_pointer(ctx, parts);
assert(parts_val != NULL /* Should not be null, we moved it to a known position on the stack */);
if (parts_val->type != VALUE_LIST) {
if (apfl_get_type(ctx, parts) != APFL_VALUE_LIST) {
apfl_raise_const_error(ctx, APFL_RESULT_ERR, apfl_messages.not_a_list);
}
size_t parts_len = apfl_len(ctx, parts);
apfl_tostring(ctx, glue);
struct apfl_string_view glue_sv = apfl_get_string(ctx, glue);
struct list_header **parts_list = &parts_val->list;
size_t total_length = 0;
for (size_t i = 0; i < (*parts_list)->len; i++) {
for (size_t i = 0; i < parts_len; i++) {
// Convert values to strings, if necessary
if (!VALUE_IS_A((*parts_list)->items[i], APFL_VALUE_STRING)) {
apfl_stack_must_push(ctx, (*parts_list)->items[i]);
apfl_get_list_member_by_index(ctx, parts, i);
if (apfl_get_type(ctx, -1) == APFL_VALUE_STRING) {
total_length += apfl_get_string(ctx, -1).len;
apfl_drop(ctx, -1);
} else {
apfl_tostring(ctx, -1);
if (!apfl_list_splice(
&ctx->gc,
parts_list,
i,
1,
stack_get_pointer(ctx, -1),
1
)) {
goto error;
}
apfl_stack_drop(ctx, -1);
total_length += apfl_get_string(ctx, -1).len;
apfl_set_list_member_by_index(ctx, -3, i, -1);
}
struct apfl_string_view part_sv;
assert(get_string_view_of_value(&part_sv, (*parts_list)->items[i]) /* Value should be a string at this point */);
total_length += part_sv.len;
if (i > 0) {
total_length += glue_sv.len;
}
@ -1332,19 +1349,20 @@ apfl_join_strings(apfl_ctx ctx, apfl_stackidx glue, apfl_stackidx parts)
goto error;
}
for (size_t i = 0; i < (*parts_list)->len; i++) {
for (size_t i = 0; i < parts_len; i++) {
if (i > 0) {
if (!apfl_string_builder_append(&sb, glue_sv)) {
goto error;
}
}
struct apfl_string_view part_sv;
assert(get_string_view_of_value(&part_sv, (*parts_list)->items[i]) /* Value should be a string at this point */);
apfl_get_list_member_by_index(ctx, parts, i);
if (!apfl_string_builder_append(&sb, part_sv)) {
if (!apfl_string_builder_append(&sb, apfl_get_string(ctx, -1))) {
goto error;
}
apfl_drop(ctx, -1);
}