4
\$\begingroup\$

My team has been given care of an old embedded C project. Its main part is a security critical RPC API and I'm trying to create a new design of its internal interfaces to make it more robust against the inevitable bugs. My main goals are:

  • Make it easy to write memory safe code (no buffer overflows, information disclosure, etc.)
  • Make error propagation hard to forget or at least easy to audit.
  • [Anything which is important for security given time constrained developers / auditors.]

My biggest joker is that because of it being an RPC server nearly all allocations have a lifetime of at most one request and the server is (currently) single threaded only. Thus I have written a per-request memory pool:

struct req_scoped_alloc_pool {
    unsigned char *base_pointer;
    size_t capacity;
    size_t bytes_used;
}
struct req_scoped_alloc_pool global_alloc_pool;


int init(…) {
    // …
    size_t GLOBAL_REQ_ALLOC_POOL_CAPACITY = 2048;
    void *alloc_res = malloc(GLOBAL_REQ_ALLOC_POOL_CAPACITY)
    if(alloc_res == NULL) {
        return SOME_ERROR_CODE
    }
    global_alloc_pool.base_pointer = (unsigned char *)alloc_res;
    global_alloc_pool.capacity = GLOBAL_REQ_ALLOC_POOL_CAPACITY;
    global_alloc_pool.bytes_used = 0;
    // …
    return 0;
}

void erase_req_allocs() {
    mem_clear(global_alloc_pool.base_pointer, global_alloc_pool.capacity);
    global_alloc_pool.bytes_used = 0;
}

void assert_empty_global_alloc_pool(HDL a_handle) {
    if(global_alloc_pool.bytes_used != 0) {
        // some more severe error reporting using RPC handle
    }
    cleanup:
}

Each remotely callable function checks the state of the global pool beforehand and clears it afterwards:

int some_external_function(HDL rpc_handle) {
    assert_empty_global_alloc_pool(rpc_handle);
    // do stuff
    set_result_values(rpc_handle, …) // or communicate errors
    erase_req_allocs();
}

Plain pointers will be replaced with fat pointers. This will make passing around pointer size information the comfortable default case. (Currently there are a lot of cases where the caller has to know how large the allocated memory for the passed pointer has to be and there are neither checks whether they get it right nor alarms when the called function is being refactored to require more memory):

typedef struct u8_vector {
    unsigned char *base_pointer;
    size_t capacity; // naming inspired by C++ vector
    size_t size; // naming inspired by C++ vector
} u8_vector

u8_vector empty_u8_vector() {
    u8_vector res;uiae
    res.base_pointer = NULL;
    res.capacity = 0;
    res.size = 0;
}

Regarding error propagation I had to compromise. Ideally I would like each function to return a struct which contains the error code and a payload. The payload is then unwrapped to a local variable via a macro, which will also check the error code. This way, to access the payload you would always check the error code. Unfortunately I haven't been able to design a solution with sufficient usability without having a C++ like auto keyword available. I thus compromised to directly allow payload structs as return values and store the error code in a global variable:

int global_error = 0;
#define RAISE_ERROR(err_code) {global_error = err_code; goto cleanup;}
#define CHECK_ERROR {if(global_error != 0) {goto cleanup;}}

Request scoped memory allocations use this global error code:

u8_vector req_malloc_u8(size_t vec_capacity) {
    u8_vector res = empty_u8_vector(); // default to empty vector if callee forgets error handling
    // Round up to multiples of 8 bytes for simple data structure alignment
    int remainder = vec_capacity % 8;
    if(remainder != 0) {
        vec_capacity += 8 - remainder;
    }
    if(global_alloc_pool.bytes_used + vec_capacity > global_alloc_pool.capacity) {
        RAISE_ERROR(SOME_ERROR_CODE);
    }
    res.base_pointer = global_alloc_pool.base_pointer + global_alloc_pool.capacity;
    res.capacity = vec_capacity;
    res.size = 0;

    cleanup:
    return res;
}

void append_from_char_p(u8_vector *target, unsigned char* source, size_t size) {
    if(target->capacity - target->size < size) {
        RAISE_ERROR(SOME_ERROR_CODE);
    }
    target->capacity += target->size;
    memcpy(target->base_pointer + target->size, source, size);
    cleanup:
}

Now for some non-trivial usage example: Expansion of nibbles in a byte vector:

typedef struct nibble_vector {
    unsigned char *base_pointer;
    size_t capacity; // naming inspired by C++ vector
    size_t size; // naming inspired by C++ vector
} nibble_vector

nibble_vector req_malloc_nibble(size_t vec_capacity) {
    nibble_vector res;
    u8_vector u8_res = req_malloc_u8(vec_capacity); CHECK_ERROR;
    res.base_pointer = u8_res.base_pointer;
    res.capacity = u8_res.capacity;
    res.size = u8_res.size;
    cleanup:
    return res;
}

As a convention, vectors passed to functions are read-only read when they are passed as struct and read-write when passed as pointers. This hopefully greatly simplifies reading method invocations:

void push_back(nibble_vector *target, unsigned char new_nibble) {
    if(target->capacity - target->size < 1) {
        RAISE_ERROR(SOME_ERROR_CODE);
    }
    target->base_pointer[target->size] = new_nibble;
    target->size += 1;
    cleanup:
}

// Expand each byte of compressed input vector to two half bytes ("nibbles") in result vector
// Fill nibbles in input vector are left out
nibble_vector expand_bytes_to_nibbles(u8_vector compressed, unsigned char fill_nibble) {
    nibble_vector res = empty_nibble_vector;
    res = req_malloc_nibble(compressed); CHECK_ERROR;
    for(int i=0; i<compressed.size; ++i) {
        unsigned char current_nibble;
        current_nibble = (compressed.base_pointer[i] & 0xF0) >> 4);
        if(current_nibble != fill_nibble) {
            push_back(&res, current_nibble); CHECK_ERROR;
        }
        current_nibble = (compressed.base_pointer[i] & 0x0F) >> 0);
        if(current_nibble != fill_nibble) {
            push_back(&res, current_nibble); CHECK_ERROR;
        }
    }
    cleanup:
    return res;
}

Is this a good design?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.