5
\$\begingroup\$

I have been working on a small library of development utilities for some time now, incrementally improving and expanding it as I make use of it in other projects. I feel that it is fairly mature now; I've worked out enough bugs that I can say it is decently stable and I feel that it is time for a code review on what I've created so far to make sure everything is kosher.

It has an implementation of better strings and string buffers, a unit testing framework, atomic code profiling framework, cleaner assert-or-fail style checks with a customizable error logging system, and safer memory management utilities.

In any case, here's the repository: https://github.com/daltonwoodard/cdevkit

I'll also post the API's for several of the sections. There are demos of the unit testing framework and code profiling framework in the cdevkit/demos/ directory.

API:

safemem.h

#ifndef __safemem_h_
#define __safemem_h_

#include <stdlib.h>
#include <stdarg.h>

#include "check.h"
#include "cdk_stack.h"

//
// Memory allocation stats
//
#ifdef TRACK_MEM
    static unsigned long int __sm_bytes_allocated_c = 0; // current
    static unsigned long int __sm_bytes_allocated_t = 0; // total
    static unsigned long int __sm_bytes_allocated_p = 0; // peak

    #define get_bytes_allocated_current() __sm_bytes_allocated_c
    #define get_bytes_allocated_total() __sm_bytes_allocated_t
    #define get_bytes_allocated_peak() __sm_bytes_allocated_p

    #define mem_stats_add( bytes ) __sm_bytes_allocated_c += (bytes);                                       \
        __sm_bytes_allocated_t += (bytes);                                                                  \
        __sm_bytes_allocated_c > __sm_bytes_allocated_p ? __sm_bytes_allocated_p = __sm_bytes_allocated_c   \
                                                        : __sm_bytes_allocated_p += 0;

    #define mem_stats_sub( bytes ) __sm_bytes_allocated_c -= (bytes);
#endif

//
// Pushes a new pointer to the register stack and returns the pointer.
// Note that this function will be called automatically when the register
// system is being used correctly.
// @param  __pr [The current register stack.]
// @param  ptr  [Pointer to the pointer to be registered.]
// @param  ci   [Function call info, automatically provided by macro incokation.]
// @return      [The poitner being registered.]
//
void * __sm_pr_register(cdk_stack * const __pr, void ** ptr,
                        cdk_call_info const * const ci);


//
// Releases the memory from all pointers registered on the stack.
// @param __pr [The current register stack.]
//
void __sm_pr_release_all(cdk_stack * __pr);


//
// Usage macros for the safemem register.
// 
#define sm_using_register() cdk_stack * const __pr = cdk_stack_create()
#define sm_end_register() do { cdk_stack_release( (cdk_stack **)&(__pr) ); } while (0);
#define sm_register(P) __sm_pr_register(__pr, &(void *){ (P) }, call_info())
#define sm_release() cdk_stack_release_with_contents( (cdk_stack **)&(__pr) )


//
// Returns the number of bytes allocated to a given pointer.
//
// Wraps the malloc_size() method from <malloc/malloc.h>
//
size_t __mem_size(void const * const ptr, cdk_call_info const * const ci);

#define mem_size(P) __mem_size((P), call_info())


//
// Allocates (count * size) bytes of memory.
//
// Note allocate_mem_() is not meant to be called directly. Rather,
// use the allocate_mem() macro.
//
// Returns a (void *) pointer to the head of the allocated block on success,
// or NULL on failure, logging an error report to stderr.
//
void * __allocate_mem(size_t const count, size_t const size,
                      cdk_call_info const * const ci);

#define allocate_mem(COUNT, SIZE) __allocate_mem((COUNT), (SIZE), call_info())

#define rallocate_mem(COUNT, SIZE) __sm_pr_register(__pr, \
    &(void *){ __allocate_mem((COUNT), (SIZE), call_info()) }, call_info())


//
// Allocates (size) bytes of memory for (count) objects and
// initializes the allocated region to values of 0.
//
// Note calloc_mem_() is not meant to be called directly. Rather,
// use the calloc_mem() macro.
//
// Returns a (void *) pointer to the head of the allocated block on success,
// or NULL on failure, logging an error report to stderr.
//
void * __calloc_mem(size_t const count, size_t const size,
                    cdk_call_info const * const ci);

#define calloc_mem(COUNT, SIZE) __calloc_mem((COUNT), (SIZE), call_info())

#define rcalloc_mem(COUNT, SIZE) __sm_pr_register(__pr, \
    &(void *){ __calloc_mem((COUNT), (SIZE), call_info()) }, call_info())


//
// Attempts to reallocate (size) bytes of memory pointed to by (void * from).
// Note realloc_mem_() is not meant to be called directly. Rather,
// use the realloc_mem() macro.
//
// Returns a (void *) pointer to the head of the reallocated block on success,
// or NULL on failure, logging an error report to stderr.
//
// If the reallocation fails, the memory pointed to by (void * ptr) is free'd.
//
void * __realloc_mem(void * ptr, size_t const size,
                     cdk_call_info const * const ci);

#define realloc_mem(FROM, SIZE) __realloc_mem((FROM), (SIZE), call_info())

#define rrealloc_mem(FROM, SIZE) __sm_pr_register(__pr, \
    &(void *){ __realloc_mem((FROM), (SIZE), call_info()) }, call_info())


//
// Allocates (size) bytes of memory on a page boundary.
//
// Note valloc_mem_() is not meant to be called directly. Rather,
// use the valloc_mem() macro.
//
// Returns a (void *) pointer to the head of the allocated block on success,
// or NULL on failure, logging an error report to stderr.
//
void * __valloc_mem(size_t const size, cdk_call_info const * const ci);

#define valloc_mem(SIZE) __valloc_mem((SIZE), call_info())

#define rvalloc_mem(SIZE) __sm_pr_register(__pr,               \
    &(void *){__valloc_mem((SIZE), call_info()) }, call_info())


//
// Copies (size) bytes of memory counted from the head of the source (from) 
// to the destination (to).
// 
// Returns a (void *) pointer to the head of the copied block,
// i.e. the original (to).
//
void * __cpy_mem(void * to, void const * const from, 
        size_t const size, cdk_call_info const * const ci);

#define cpy_mem(T, F, S) __cpy_mem( (T), (F), (S), call_info() )


//
// Returns a (deep) copy of the given memory; i.e. a non-aliased clone.
//
void * __cpy_of(void const * const ptr, cdk_call_info const * const ci);

#define cpy_of(P) __cpy_of((P), call_info())

#define rcpy_of(P) __sm_pr_register(__pr,                          \
    &(void *){ __cpy_of((P), call_info()) }, call_info())


//
// Returns a (deep) copy of the given memory up to the given length;
// i.e. a non-aliased clone.
//
void * __cpyn_of(void const * const ptr, size_t const len,
                 cdk_call_info const * const ci);

#define cpyn_of(P, N) __cpyn_of((P), (N), call_info() )

#define rcpyn_of(P, N) __sm_pr_register(__pr,                          \
    &(void *){ __cpyn_of((P), (N), call_info()) }, call_info())


//
// Clears all memory allocated to the given pointer,
// setting all bytes to value '\0'.
//
void __clear_mem(void * const ptr, cdk_call_info const * const ci);

#define clear_mem(P) __clear_mem((P), call_info())


//
// Sets all memory allocated to the given pointer, 
// setting all bytes to the given value.
//
void __set_mem(void * const ptr, char const val,
               cdk_call_info const * const ci);

#define set_mem(P, V) __set_mem((P), (V), call_info())


//
// Checks whether the given pointer is NULL valued; 
// i.e. an invalid (or dangling) pointer.
//
// Note, __verify_pointer() is not meant to be called directly. 
// Rather, use the verify_pointer() macro.
//
// Returns 0 if the pointer is valid, -1 if it is invalid.
//
bool __verify_pointer(void const ** const ptr,
                      cdk_call_info const * const ci);

#define verify_pointer(PTR) __verify_pointer( &(void *){ (PTR) }, call_info() )


//
// Free's the memory allocated to the given pointer.
//
// Note, __free_mem() is not meant to be called directly. 
// Rather, use the free_mem() macro.
//
// Upon success, the return value is void. If the pointer passed was NULL 
// valued, then a warning is logged to stderr.
//
void __free_mem(void ** ptr, cdk_call_info const * const ci);

#define free_mem(PTR) __free_mem( &(void *){ (PTR) }, call_info() )


//
// Calls the dedicated object (or data structure, or struct) desctructor 
// and then free's memory allocated to the object (or data strucutre, 
// or struct) itself.
//
// Note, __free_obj() is not meant to be called directly. 
// Rather, use the free_obj() macro.
//
// Upon success, the return value is void. If the pointer passed was
// NULL valued, then a warning is logged to stderr. The caller's destructor 
// function is expected to return void, and errors within it's own scope are 
// not expected to be propogated upwards to free_obj_().
// 
// Note also that the caller's destructor method IS NOT expected to 
// deallocate the object itself; rather, only the memory allocated to 
// any of it's contents. If, however, this behavior cannot be avoided
// the given pointer MUST be set to NULL, otherwise free_obj_() will fail 
// on attempt to deallocate. To allow for this, the destructor is passed a
// (void **) pointer that can be dereferenced to set the pointer itself 
// to NULL.
//
void __free_obj(void ** ptr, void (* destructor)(void **),
                cdk_call_info const * const ci);

#define free_obj(OBJ, DST) __free_obj(&(void *){ (OBJ) }, (DST), call_info())

#endif

cstring.h

#ifndef __cstring_h_
#define __cstring_h_

#include <stdlib.h>
#include <stdarg.h>
#include <stdbool.h>
#include <string.h>
#include "safemem.h"
#include "check.h"


// The standard cstring is an immutable
// NULL-terminated char const array with
// length information included.
typedef struct {
    size_t const len;
    char const * const str_val;
} cstring;

// The cstringbuffer is a mutable
// and resizeable NULL-terminated char array.
typedef struct {
    size_t len;
    size_t capacity;
    char * str_val;
} cstringbuffer;


#define as_string( s ) (s)->str_val
#define cstring_len( s ) (s)->len;


//
// Returns a new cstring with the given literal value.
// Note that __create_cstring() should not be called direcly. Rather,
// the new_cstring() macro should be used.
// If sufficient memory cannot be allocaed, the return value is NULL.
//
cstring * __create_cstring(char const * const str_val);

#define new_cstring( s ) __create_cstring( (s) )


//
// Returns a new cstring with the result of the formatted input.
// Note that __create_cstring() should not be called direcly. Rather,
// the new_cstring() macro should be used.
// If sufficient memory cannot be allocaed, the return value is NULL.
//
cstring * __fcreate_cstring(char const * const fmt, ...);

#define new_fcstring( f, ... ) __fcreate_cstring( (f), ##__VA_ARGS__ )


//
// Releases the memory allocated to the given cstring and sets the pointer to NULL.
//
void __cstring_release(cstring ** s, cdk_call_info const * const ci);

#define cstring_release( s ) __cstring_release( (s), call_info() )


//
// Creates a new cstringbuffer.
//
cstringbuffer * __create_cstringbuffer(size_t const capacity);

#define new_cstringbuffer() __create_cstringbuffer(0) // We let the method use the default capacity by passing in zero
#define new_cstringbuffer_wcap( c ) __create_cstringbuffer( (c) )


//
// Releases the memory allocated to the given cstringbuffer and sets the pointer to NULL.
//
void __cstringbuffer_release(cstringbuffer ** b, cdk_call_info const * const ci);

#define cstringbuffer_release( b ) __cstringbuffer_release( (b), call_info() )


//
// Ensures that the given cstringbuffer has the given capacity.
// Returns 1 on success, or 0 on failure. If the memory cannot be
// allocated, the contents of the original cstringbuffer are left
// unchanged.
//
int cstringbuffer_ensure_capacity(cstringbuffer * const b, size_t const capacity);


//
// Returns a copy of the given cstring as a cstringbuffer, or
// NULL on failure. In either case, the contents of the
// original cstring are left unchanged.
//
cstringbuffer * cstring_to_buffer(cstring const * const s);


//
// Returns a copy of the given cstringbuffer as a cstring, or
// NULL on failure. In either case, the contents of the original
// cstringbuffer are left unchanged.
//
cstring * cstringbuffer_to_string(cstringbuffer const * const b);


//
// Writes the single given char to the string buffer. Returns,
// 1 on success, or 0 on failure. In the case of failure, the contents
// of the original cstringbuffer are left unchanged.
//
int cstringbuffer_write_char(cstringbuffer * const b, char const c);


//
// Appends a copy of the string value of the given cstring to the given cstringbuffer.
// Returns 1 on success, or 0 on failure. In the case of failure, the contents of the
// original cstringbuffer are left unchanged.
//
int cstringbuffer_append(cstringbuffer * const b, cstring const * const s);


//
// Appends a copy of the string value of the given string pointed to by 'str' to the
// given cstringbuffer. Returns 1 on success, or 0 on failure. In the case of failure,
// the contents of the original cstringbuffer are left unchanged.
//
int __cstringbuffer_append_allocated_source(cstringbuffer * const b, char const * const str, size_t const len);

#define cstringbuffer_append_alloced( b, s, l ) __cstringbuffer_append_allocated_source( (b), (s), (l) )


//
// Appends a copy of the string value of the given string literal to the given cstringbuffer.
// Returns 1 on success, or 0 on failure. In the case of failure, the contents of the
// original cstringbuffer are left unchanged.
//
int __cstringbuffer_append_literal(cstringbuffer * const b, char const * const l, size_t const len);

#define cstringbuffer_append_literal( b, l ) __cstringbuffer_append_literal( (b), (l), sizeof((l)) )


//
// Takes a format string and a variadic list of arguments to append to the given cstringbuffer.
// Returns 1 on success, or 0 on failure. In the case of failure, the contents of the
// original cstringbuffer are left unchanged.
//
int __cstringbuffer_append_format(cstringbuffer * const b, char const * const fmt, ...);

#define cstringbuffer_append_format( b, f, ... ) __cstringbuffer_append_format( (b), (f), ##__VA_ARGS__ )


//
// Returns a dedicated copy of the given cstring, or
// NULL if sufficient memory cannot be allocated. In either
// case, the contents of the original cstring are left unchanged.
//
cstring * cstring_clone(cstring const * const s);


//
// Returns a dedicated copy of the given cstringbuffer, or
// NULL if sufficient memory cannot be allocated. In either
// case, the contents of the original cstringbuffer are left unchanged.
// 
cstringbuffer * cstringbuffer_clone(cstringbuffer const * const b); 


//
// Returns a new cstring whose contents consist of the string value of the
// first argument concatenated with that of the second, or NULL on failure.
// In either case, the contents of both arguments are left unchanged.
//
cstring * cstring_concat(cstring const * const s, cstring const * const with);


//
// Returns a new cstring constructed from the variadic list of cstrings passed to it,
// or NULL on failure. In either case, the contents of all cstrings passed in are left
// unchanged. Note that if a single cstring is passed in, the behavior is identical to
// that of cstring_clone. Note also that if two cstrings are passed, the behavior is
// identical to that of cstring_concat.
//
cstring * cstring_join(size_t const num, ...);


//
// Returns a copy of the provided cstring's value as a normal char array.
// If sufficient memory cannot be allocated, the return value is NULL. In
// either case, the contents of the cstring are left unchanged.
//
char * cstring_to_char_array(cstring const * const s);


//
// Returns a copy of the provided cstringbuffer's value as a normal char array.
// If sufficient memory cannot be allocated, the return value is NULL. In
// either case, the contents of the cstringbuffer are left unchanged.
// 
char * cstringbuffer_to_char_array(cstringbuffer const * const b); 


//
// Returns a copy of the substring of the given cstring indicated by the given indices.
// If sufficient memory could not be allocated, the return value is NULL. In
// either case, the contents of the cstring are left unchanged.
//
cstring * cstring_substring(cstring const * const s, size_t const from, size_t const to);


//
// Returns the first index from the left of the given char in the given cstring,
// or -1 if not present.
//
int char_indexl(cstring const * const s, char const c);


//
// Returns the first index from the left of the given char in the given cstringbuffer,
// or -1 if not present.
//
int b_char_indexl(cstringbuffer const * const s, char const c);


//
// Returns the first index from the right of the given char in the given cstring,
// or -1 if not present.
//
int char_indexr(cstring const * const s, char const c);


//
// Returns the first index from the right of the given char in the given cstringbuffer,
// or -1 if not present.
//
int b_char_indexr(cstringbuffer const * const s, char const c);


//
// Returns the count of how many times the given char occurs in the given cstring.
//
int count_char_occurrence(cstring const * const s, char const c);


//
// Returns the count of how many times the given char occurs in the given cstringbuffer.
//
int b_count_char_occurrence(cstringbuffer const * const s, char const c);


//
// Returns a copy of the given cstring, with all alphabetic characters converted
// to upper case. If sufficient memory could not be allocated, the return value is NULL.
// In either case, the contents of the cstring are left unchanged.
//
cstring * cstring_toupper(cstring const * const s);


//
// Returns a copy of the given cstring, with all alphabetic characters converted
// to lower case. If sufficient memory could not be allocated, the return value is NULL.
// In either case, the contents of the cstring are left unchanged.
//
cstring * cstring_tolower(cstring const * const s);


//
// Returns the index of the first occurence of the substring in the given cstring, or -1 if not found.
//
int cstrstr(cstring const * const haystack, cstring const * const needle);


//
// Returns the index of the first occurence of the substring in the given cstring, or -1 if not found.
//
int cstrstrl(cstring const * const haystack, char const * const needle);


//
// Returns the index of the first occurence of the substring in the given cstringbuffer, or -1 if not found.
//
int cstrbstr(cstringbuffer const * const haystack, cstring const * const needle);


//
// Returns the index of the first occurence of the substring in the given cstringbuffer, or -1 if not found.
//
int cstrbstrl(cstringbuffer const * const haystack, char const * const needle);


//
// Returns the lexicographic comparison value between the given cstrings.
//
int cstring_cmp(void const * const a, void const * const b);


//
// Returns the lexicographic comparison value between the given cstrings,
// ignoring case; i.e., "hello", "HELLO", and "HeLlO" all compare to 0.
//
int cstring_cmp_ncase(void const * const a, void const * const b);


//
// Returns the lexicographic comparison value between the given cstringbuffers.
//
int cstringbuffer_cmp(void const * const a, void const * const b);


//
// Returns the lexicographic comparison value between the given cstringbuffers,
// ignoring case; i.e., "hello", "HELLO", and "HeLlO" all compare to 0.
//
int cstringbuffer_cmp_ncase(void const * const a, void const * const b);


#endif

Unfortunately I can't add anything else due to character limits, but the remainder of the project is in the repo I linked.

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

Standards

Note also that this project is developed to be compliant with the C11 standard.

Cool, let's find non-standard behavior.

The first thing that jumps out to me is your usage of double underscores.

Both C99 and C11 say (in section 7.1.3):

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

You include <malloc/malloc.h> everywhere. The only operating system where I could find that header file is MacOS. This is definitely not standard.

You seem to be very dependent on the function malloc_size in many places. As far as I know, only MacOS implements it.

You use the function valloc. This is a non-standard function that has been marked as obsolete in 4.3 BSD in 1986. You can get this functionality from a standard (not C, but at least POSIX) by using the posix_memalign function.

Problematic behavior

In cstring_cmp and cstringbuffer_cmp you compare with the shortest string with strncmp which means that you're only comparing the prefixes of the strings (I think, I haven't dug into the exact details of the library) and why not just use memcmp since you're not comparing C strings anyway.

It's possible to write \0 into a cstringbuffer with cstringbuffer_write_char (and other functions) that will update the length to include that and later append further characters at the end of the buffers, but you're then using normal strcmp, strncpy, etc. to compare and copy the strings which will not do what you want. Either disallow \0 explicitly or use functions that don't get confused by it.

allocate_mem has a good prototype with count and size. This is just like calloc and it's what a modern memory allocator should do. But, you miss the point of why calloc does it. The main reason is that you can detect overflows when multiplying count * size. You don't check for overflow in that multiplication which can lead to security issues.

Matters of taste

You're always allocating a struct on the stack, then malloc the same struct, then return a copy. Why? It seems like extra work.

Macros with magic goto error; with the error: label being outside of the macro are horrifying.

You do do { ... } while (0) in macros, this is good style. But you then defeat half the purpose of building macros this way by adding the ; inside the macro. for example if (foo) log_error("foo"); else do_something(); will not compile.

\$\endgroup\$
1
\$\begingroup\$

I've only looked (briefly) at the cstring.h part so far. I like it for the most part. It looks like it could help projects avoid the chaos and subtle error that often creep into C code that uses lots of character arrays.

Suggestions:

  • You seem to only use dynamic (heap) memory. Particularly in embedded code, there are times when using the heap is just a bad idea. What about support for stack-based strings? If I make a character array of my own, I'd like a way to give you the address and size, and get back a wrapped cstring that works with the rest of the API. Let the calling code manage the memory allocation and the library manage the string manipulation.
  • ints are for counting and math. int is a poor way to model "1 on success, or 0 on failure". You don't want the calling code using the return values for counting or math, so you shouldn't choose a return type that encourages it. this context, 0 and 1 are magic numbers. You could use bool instead, or define your own enumeration.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.