From 20d3e6f4e1897ae1896fd82262d1efaeaf01ba34 Mon Sep 17 00:00:00 2001 From: Daniel Agar Date: Thu, 23 Dec 2021 17:20:28 -0500 Subject: [PATCH] parameters: add simple BSON verification pass after export --- src/lib/parameters/CMakeLists.txt | 1 + src/lib/parameters/parameters.cpp | 127 +++++++++++++++++++++-- src/lib/parameters/tinybson/tinybson.cpp | 16 ++- src/systemcmds/param/param.cpp | 2 +- src/systemcmds/tests/test_parameters.cpp | 2 +- 5 files changed, 126 insertions(+), 22 deletions(-) diff --git a/src/lib/parameters/CMakeLists.txt b/src/lib/parameters/CMakeLists.txt index b8f91e41be..26e909596f 100644 --- a/src/lib/parameters/CMakeLists.txt +++ b/src/lib/parameters/CMakeLists.txt @@ -171,6 +171,7 @@ if (NOT "${PX4_BOARD}" MATCHES "px4_io") target_compile_definitions(parameters PRIVATE -DMODULE_NAME="parameters") target_compile_options(parameters PRIVATE + #-DDEBUG_BUILD -Wno-cast-align # TODO: fix and enable -Wno-sign-compare # TODO: fix and enable ) diff --git a/src/lib/parameters/parameters.cpp b/src/lib/parameters/parameters.cpp index 482af68884..73a1478c92 100644 --- a/src/lib/parameters/parameters.cpp +++ b/src/lib/parameters/parameters.cpp @@ -1183,7 +1183,7 @@ static int param_save_file_internal(const char *filename) while (res != OK && attempts > 0) { // write parameters to file - int fd = ::open(filename, O_WRONLY | O_CREAT, PX4_O_MODE_666); + int fd = ::open(filename, O_RDWR | O_CREAT, PX4_O_MODE_666); if (fd > -1) { res = param_export(fd, nullptr); @@ -1272,10 +1272,111 @@ param_load_default() return res; } +static int param_verify_callback(bson_decoder_t decoder, bson_node_t node) +{ + if (node->type == BSON_EOO) { + return 0; + } + + // find the parameter this node represents + param_t param = param_find_no_notification(node->name); + + if (param == PARAM_INVALID) { + PX4_ERR("verify: invalid parameter '%s'", node->name); + return -1; + } + + // handle verifying the parameter from the node + switch (node->type) { + case BSON_INT32: { + if (param_type(param) != PARAM_TYPE_INT32) { + PX4_ERR("verify: invalid param type %d for '%s' (BSON_INT32)", param_type(param), node->name); + return -1; + } + + int32_t value; + + if (param_get(param, &value) == 0) { + if (value == node->i32) { + return 1; // valid + + } else { + PX4_ERR("verify: '%s' invalid BSON value %" PRIi32 "(expected %" PRIi32 ")", node->name, node->i32, value); + } + } + } + break; + + case BSON_DOUBLE: { + if (param_type(param) != PARAM_TYPE_FLOAT) { + PX4_ERR("verify: invalid param type %d for '%s' (BSON_DOUBLE)", param_type(param), node->name); + return -1; + } + + float value; + + if (param_get(param, &value) == 0) { + if (fabsf(value - (float)node->d) <= FLT_EPSILON) { + return 1; // valid + + } else { + PX4_ERR("verify: '%s' invalid BSON value %.3f (expected %.3f)", node->name, node->d, (double)value); + } + } + } + break; + + default: + PX4_ERR("verify: '%s' invalid node type %d", node->name, node->type); + } + + return -1; +} + +static int param_verify(int fd) +{ + if (fd < 0) { + return -1; + } + + if (lseek(fd, 0, SEEK_SET) != 0) { + PX4_ERR("verify: seek failed"); + return -1; + } + + bson_decoder_s decoder{}; + + if (bson_decoder_init_file(&decoder, fd, param_verify_callback) == 0) { + int result = -1; + + do { + result = bson_decoder_next(&decoder); + + } while (result > 0); + + if (result == 0) { + return 0; + + } else if (result == -ENODATA) { + PX4_ERR("verify: no BSON data"); + + } else { + PX4_ERR("verify: failed (%d)", result); + } + } + + return -1; +} + int param_export(int fd, param_filter_func filter) { - int result = -1; + /* no modified parameters -> we are done */ + if (param_values == nullptr) { + return 0; + } + + int result = -1; perf_begin(param_export_perf); if (fd < 0) { @@ -1303,11 +1404,9 @@ param_export(int fd, param_filter_func filter) param_lock_reader(); uint8_t bson_buffer[256]; - bson_encoder_init_buf_file(&encoder, fd, &bson_buffer, sizeof(bson_buffer)); - /* no modified parameters -> we are done */ - if (param_values == nullptr) { - result = 0; + if (bson_encoder_init_buf_file(&encoder, fd, &bson_buffer, sizeof(bson_buffer)) != 0) { + result = -1; goto out; } @@ -1323,7 +1422,7 @@ param_export(int fd, param_filter_func filter) param_get_default_value_internal(s->param, &default_value); if (s->val.i == default_value) { - PX4_DEBUG("skipping %s %d export", param_name(s->param), default_value); + PX4_DEBUG("skipping %s %" PRIi32 " export", param_name(s->param), default_value); continue; } } @@ -1348,7 +1447,7 @@ param_export(int fd, param_filter_func filter) switch (param_type(s->param)) { case PARAM_TYPE_INT32: { const int32_t i = s->val.i; - PX4_DEBUG("exporting: %s (%d) size: %lu val: %d", name, s->param, (long unsigned int)size, i); + PX4_DEBUG("exporting: %s (%d) size: %lu val: %" PRIi32, name, s->param, (long unsigned int)size, i); if (bson_encoder_append_int(&encoder, name, i) != 0) { PX4_ERR("BSON append failed for '%s'", name); @@ -1379,12 +1478,18 @@ out: if (result == 0) { if (bson_encoder_fini(&encoder) == PX4_OK) { - if (!filter) { - params_unsaved.reset(); + // verify exported bson + if (param_verify(fd) == 0) { + if (!filter) { + params_unsaved.reset(); + } + + } else { + result = -1; } } else { - PX4_ERR("BSON encoder finialize failed"); + PX4_ERR("BSON encoder finalize failed"); result = -1; } } diff --git a/src/lib/parameters/tinybson/tinybson.cpp b/src/lib/parameters/tinybson/tinybson.cpp index 8280dfcc25..8a475b9db4 100644 --- a/src/lib/parameters/tinybson/tinybson.cpp +++ b/src/lib/parameters/tinybson/tinybson.cpp @@ -517,23 +517,21 @@ bson_encoder_fini(bson_encoder_t encoder) } // record document size - if (encoder->fd > -1) { - if (lseek(encoder->fd, 0, SEEK_SET) == 0) { - debug("writing document size %" PRIi32 " to beginning of file", encoder->total_document_size); + debug("writing document size %" PRIi32, encoder->total_document_size); + const int32_t bson_doc_bytes = encoder->total_document_size; - if (::write(encoder->fd, &encoder->total_document_size, - sizeof(encoder->total_document_size)) != sizeof(encoder->total_document_size)) { + if (encoder->fd > -1) { + if ((lseek(encoder->fd, 0, SEEK_SET) != 0) + || (::write(encoder->fd, &bson_doc_bytes, sizeof(bson_doc_bytes)) != sizeof(bson_doc_bytes))) { - CODER_KILL(encoder, "write error on document length"); - } + CODER_KILL(encoder, "write error on document length"); } ::fsync(encoder->fd); } else if (encoder->buf != nullptr) { /* update buffer length */ - int32_t len = bson_encoder_buf_size(encoder); - memcpy(encoder->buf, &len, sizeof(len)); + memcpy(encoder->buf, &bson_doc_bytes, sizeof(bson_doc_bytes)); } return 0; diff --git a/src/systemcmds/param/param.cpp b/src/systemcmds/param/param.cpp index 3031adfbee..1422c6fe71 100644 --- a/src/systemcmds/param/param.cpp +++ b/src/systemcmds/param/param.cpp @@ -434,7 +434,7 @@ static int do_save(const char *param_file_name) { /* create the file */ - int fd = open(param_file_name, O_WRONLY | O_CREAT, PX4_O_MODE_666); + int fd = open(param_file_name, O_RDWR | O_CREAT, PX4_O_MODE_666); if (fd < 0) { PX4_ERR("open '%s' failed (%i)", param_file_name, errno); diff --git a/src/systemcmds/tests/test_parameters.cpp b/src/systemcmds/tests/test_parameters.cpp index 23472208d7..be7fa1063c 100644 --- a/src/systemcmds/tests/test_parameters.cpp +++ b/src/systemcmds/tests/test_parameters.cpp @@ -401,7 +401,7 @@ bool ParameterTest::exportImportAll() // backup current parameters const char *param_file_name = PX4_STORAGEDIR "/param_backup"; - int fd = open(param_file_name, O_WRONLY | O_CREAT, PX4_O_MODE_666); + int fd = open(param_file_name, O_RDWR | O_CREAT, PX4_O_MODE_666); if (fd < 0) { PX4_ERR("open '%s' failed (%i)", param_file_name, errno);