From 2d3f0b19a975268a42e0b82d90e8c39d9c3a99b9 Mon Sep 17 00:00:00 2001 From: Hayden Roche Date: Tue, 12 Dec 2023 12:50:46 -0800 Subject: [PATCH] Limit scope of libc usage in note-c. (#126) --- .github/workflows/ci.yml | 19 ++++++ CMakeLists.txt | 11 ++++ n_cjson.c | 29 +++++++-- scripts/check_libc_dependencies.sh | 96 +++++++++++++++++++++++++++++ test/CMakeLists.txt | 2 + test/include/test_static.h | 1 + test/src/JPrintUnformatted_test.cpp | 66 ++++++++++++++++++++ test/src/Jtolower_test.cpp | 73 ++++++++++++++++++++++ 8 files changed, 291 insertions(+), 6 deletions(-) create mode 100755 scripts/check_libc_dependencies.sh create mode 100644 test/src/JPrintUnformatted_test.cpp create mode 100644 test/src/Jtolower_test.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ac423257..a6dfdeef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,6 +79,25 @@ jobs: run: | docker run --rm --volume $(pwd):/note-c/ --workdir /note-c/ --entrypoint ./scripts/build_docs.sh ghcr.io/blues/note_c_ci:latest + check_libc_dependencies: + runs-on: ubuntu-latest + if: ${{ always() }} + needs: [build_ci_docker_image] + + steps: + - name: Checkout code + uses: actions/checkout@v3 + + - name: Load CI Docker image + # Only load the Docker image artifact if build_ci_docker_image actually + # ran (e.g. it wasn't skipped and was successful). + if: ${{ needs.build_ci_docker_image.result == 'success' }} + uses: ./.github/actions/load-ci-image + + - name: Check note-c's libc dependencies + run: | + docker run --rm --volume $(pwd):/note-c/ --workdir /note-c/ --entrypoint ./scripts/check_libc_dependencies.sh ghcr.io/blues/note_c_ci:latest + run_unit_tests: runs-on: ubuntu-latest if: ${{ always() }} diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ea02f1a..a0e64bfb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -18,6 +18,7 @@ option(NOTE_C_BUILD_TESTS "Build tests." OFF) option(NOTE_C_COVERAGE "Compile for test NOTE_C_COVERAGE reporting." OFF) option(NOTE_C_MEM_CHECK "Run tests with Valgrind." OFF) option(NOTE_C_BUILD_DOCS "Build docs." OFF) +option(NOTE_C_NO_LIBC "Build the library without linking against libc, generating errors for any undefined symbols." OFF) set(NOTE_C_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR}) add_library(note_c SHARED) @@ -56,6 +57,16 @@ target_include_directories( PUBLIC ${NOTE_C_SRC_DIR} ) +if(NOTE_C_NO_LIBC) + target_link_options( + note_c + PRIVATE + -nostdlib + -nodefaultlibs + LINKER:--no-undefined + ) +endif() + if(NOTE_C_BUILD_TESTS) # Including CTest here rather than in test/CMakeLists.txt allows us to run # ctest from the root build directory (e.g. build/ instead of build/test/). diff --git a/n_cjson.c b/n_cjson.c index 71d9f01a..00ec67d0 100644 --- a/n_cjson.c +++ b/n_cjson.c @@ -82,6 +82,12 @@ #define PRINT_TAB_CHARS 4 +#ifdef NOTE_C_TEST +#include "test_static.h" +#else +#define NOTE_C_STATIC static +#endif + typedef struct { const unsigned char *json; size_t position; @@ -115,6 +121,17 @@ N_CJSON_PUBLIC(const char*) JVersion(void) return NOTE_C_STRINGIZE(N_CJSON_VERSION_MAJOR) "." NOTE_C_STRINGIZE(N_CJSON_VERSION_MINOR) "." NOTE_C_STRINGIZE(N_CJSON_VERSION_PATCH); } +NOTE_C_STATIC char Jtolower(char c) +{ + if (c < 'A' || c > 'Z') { + return c; + } + + // 32 is the distance between any ASCII uppercase letter and its lowercase + // counterpart. + return c + 32; +} + /* Case insensitive string comparison, doesn't consider two NULL pointers equal though */ static int case_insensitive_strcmp(const unsigned char *string1, const unsigned char *string2) { @@ -126,13 +143,13 @@ static int case_insensitive_strcmp(const unsigned char *string1, const unsigned return 0; } - for(; tolower(*string1) == tolower(*string2); (void)string1++, string2++) { + for(; Jtolower(*string1) == Jtolower(*string2); (void)string1++, string2++) { if (*string1 == '\0') { return 0; } } - return tolower(*string1) - tolower(*string2); + return Jtolower(*string1) - Jtolower(*string2); } static unsigned char* Jstrdup(const unsigned char* string) @@ -417,7 +434,7 @@ static Jbool print_number(const J * const item, printbuffer * const output_buffe /* This checks for NaN and Infinity */ if ((d * 0) != 0) { char *nbuf = (char *) number_buffer; - strcpy(nbuf, "null"); + strlcpy(nbuf, "null", JNTOA_MAX); length = strlen(nbuf); } else { #if !CJSON_NO_CLIB @@ -1160,7 +1177,7 @@ static Jbool print_value(const J * const item, printbuffer * const output_buffer if (output == NULL) { return false; } - strcpy((char*)output, c_null); + strlcpy((char*)output, c_null, c_null_len+1); return true; case JFalse: @@ -1168,7 +1185,7 @@ static Jbool print_value(const J * const item, printbuffer * const output_buffer if (output == NULL) { return false; } - strcpy((char*)output, c_false); + strlcpy((char*)output, c_false, c_false_len+1); return true; case JTrue: @@ -1176,7 +1193,7 @@ static Jbool print_value(const J * const item, printbuffer * const output_buffer if (output == NULL) { return false; } - strcpy((char*)output, c_true); + strlcpy((char*)output, c_true, c_true_len+1); return true; case JNumber: diff --git a/scripts/check_libc_dependencies.sh b/scripts/check_libc_dependencies.sh new file mode 100755 index 00000000..a54de7f5 --- /dev/null +++ b/scripts/check_libc_dependencies.sh @@ -0,0 +1,96 @@ +#!/bin/bash + +SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) +ROOT_SRC_DIR="$SCRIPT_DIR/.." + +if [[ ! -f "$ROOT_SRC_DIR/CMakeLists.txt" ]]; then + echo "Failed to find note-c root directory. (did the location of check_libc_dependencies.sh change?)" + exit 1 +fi + +pushd $ROOT_SRC_DIR $@ > /dev/null + +CMAKE_OPTIONS="-DNOTE_C_NO_LIBC=1" + +cmake -B build/ $CMAKE_OPTIONS +if [[ $? -ne 0 ]]; then + echo "Failed to run CMake." + popd $@ > /dev/null + exit 1 +fi + +PERMITTED_FNS=( + # The mem* functions are ok. + "memchr" + "memcmp" + "memcpy" + "memmove" + "memset" + # These string functions are ok. + "strchr" + "strcmp" + "strlen" + "strncmp" + "strstr" + # TODO: The only explicit usage of strtod is in the cJSON code if + # CJSON_NO_CLIB is NOT true. It's true by default, so we need to figure out + # why strtod is still being brought in. + "strtod" + # strtol comes from us using atoi in NoteGetEnvInt. + "strtol" + # Used by NotePrintf. + "vsnprintf" +) + +# Function to check if an element is in an array. +element_in_array() { + local element="$1" + local array=("${@:2}") + + for item in "${array[@]}"; do + if [ "$item" == "$element" ]; then + return 0 # Element found in array + fi + done + return 1 # Element not found in array +} + +BUILD_OUTPUT=$(cmake --build build/ -j 2>&1) +if [[ $? -ne 0 ]]; then + # Iterate over the lines from the build output to get all the undefined + # references. + UNDEF_REFS=() + while IFS= read -r LINE; do + PATTERN="undefined reference to \`(.*)'" + if [[ $LINE =~ $PATTERN ]]; then + UNDEF_REFS+=("${BASH_REMATCH[1]}") + fi + done <<< "$BUILD_OUTPUT" + + # Remove duplicates + UNDEF_REFS=($(printf "%s\n" "${UNDEF_REFS[@]}" | sort -u)) + + # Check if each function that caused an undefined reference error is + # permitted. + FAIL=0 + for UNDEF_REF in "${UNDEF_REFS[@]}"; do + if element_in_array "$UNDEF_REF" "${PERMITTED_FNS[@]}"; then + echo "$UNDEF_REF is permitted." + else + echo "$UNDEF_REF is NOT permitted" + FAIL=1 + fi + done + + if [ "$FAIL" -eq 1 ]; then + echo "Unpermitted libc functions found." + popd $@ > /dev/null + exit 1 + fi +else + echo "Build unexpectedly succeeded. The build should fail because certain permitted libc functions shouldn't be found when linking." + popd $@ > /dev/null + exit 1 +fi + +popd $@ > /dev/null diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 954ff645..a6a9b3ae 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -158,6 +158,8 @@ add_test(NoteBinaryStoreEncodedLength_test) add_test(NoteBinaryStoreReceive_test) add_test(NoteBinaryStoreReset_test) add_test(NoteBinaryStoreTransmit_test) +add_test(JPrintUnformatted_test) +add_test(Jtolower_test) if(NOTE_C_COVERAGE) find_program(LCOV lcov REQUIRED) diff --git a/test/include/test_static.h b/test/include/test_static.h index 4071d06b..1f3422bc 100644 --- a/test/include/test_static.h +++ b/test/include/test_static.h @@ -18,6 +18,7 @@ void delayIO(void); const char * i2cNoteQueryLength(uint32_t * available, size_t timeoutMs); void setTime(JTIME seconds); bool timerExpiredSecs(uint32_t *timer, uint32_t periodSecs); +char Jtolower(char c); #ifdef __cplusplus } diff --git a/test/src/JPrintUnformatted_test.cpp b/test/src/JPrintUnformatted_test.cpp new file mode 100644 index 00000000..66ac45b3 --- /dev/null +++ b/test/src/JPrintUnformatted_test.cpp @@ -0,0 +1,66 @@ +/*! + * @file JPrintUnformatted_test.cpp + * + * Written by the Blues Inc. team. + * + * Copyright (c) 2023 Blues Inc. MIT License. Use of this source code is + * governed by licenses granted by the copyright holder including that found in + * the + * LICENSE + * file. + * + */ + +#ifdef NOTE_C_TEST + +#include + +#include "n_lib.h" + +namespace +{ + +// We typically don't write unit tests for cJSON functions. This is an exception +// because we modified some of the internal cJSON printing code to use strlcpy +// instead of strcpy, and we want to make sure we didn't introduce a bug. +SCENARIO("JPrintUnformatted") +{ + NoteSetFnDefault(malloc, free, NULL, NULL); + + GIVEN("A valid JSON object") { + J *jsonObj = JParse("{" + "\"string\": \"Hello, World!\"," + "\"number\": 42," + "\"boolean\": true," + "\"nullValue\": null," + "\"array\": [1, \"two\", false, 3.14]," + "\"nestedObject\": {" + "\"key1\": \"value1\"," + "\"key2\": 123," + "\"key3\": {" + "\"subKey1\": true," + "\"subKey2\": [\"apple\", \"banana\", \"cherry\"]" + "}" + "}" + "}"); + + REQUIRE(jsonObj != NULL); + + WHEN("JPrintUnformatted is called on that object") { + char *result = JPrintUnformatted(jsonObj); + + THEN("A non-NULL, non-zero length string is returned") { + REQUIRE(result != NULL); + CHECK(strlen(result) > 0); + } + + JFree(result); + } + + JDelete(jsonObj); + } +} + +} + +#endif // NOTE_C_TEST diff --git a/test/src/Jtolower_test.cpp b/test/src/Jtolower_test.cpp new file mode 100644 index 00000000..26386944 --- /dev/null +++ b/test/src/Jtolower_test.cpp @@ -0,0 +1,73 @@ +/*! + * @file Jtolower_test.cpp + * + * Written by the Blues Inc. team. + * + * Copyright (c) 2023 Blues Inc. MIT License. Use of this source code is + * governed by licenses granted by the copyright holder including that found in + * the + * LICENSE + * file. + * + */ + +#ifdef NOTE_C_TEST + +#include + +#include "n_lib.h" +#include "test_static.h" + +namespace +{ + +SCENARIO("Jtolower") +{ + const char alphabetUpper[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; + const char alphabetLower[] = "abcdefghijklmnopqrstuvwxyz"; + char result[sizeof(alphabetUpper)] = {0}; + + GIVEN("The uppercase characters of the alphabet") { + WHEN("Jtolower is called on each letter") { + for (size_t i = 0; i < sizeof(alphabetUpper); ++i) { + result[i] = Jtolower(alphabetUpper[i]); + } + + THEN("The corresponding lowercase letter is returned") { + for (size_t i = 0; i < sizeof(alphabetUpper); ++i) { + CHECK(result[i] == alphabetLower[i]); + } + } + } + } + + GIVEN("The lowercase characters of the alphabet") { + WHEN("Jtolower is called on each letter") { + for (size_t i = 0; i < sizeof(alphabetLower); ++i) { + result[i] = Jtolower(alphabetLower[i]); + } + + THEN("The same lowercase letter is returned") { + for (size_t i = 0; i < sizeof(alphabetLower); ++i) { + CHECK(result[i] == alphabetLower[i]); + } + } + } + } + + GIVEN("A non-letter char") { + char invalid = '.'; + + WHEN("Jtolower is called on that char") { + char invalidResult = Jtolower(invalid); + + THEN("The non-letter char is returned") { + CHECK(invalidResult == invalid); + } + } + } +} + +} + +#endif // NOTE_C_TEST