Safe stack abstraction

Modularization
Generally I like well modularized C programs and libraries, however, in this case the library would benefit from having all the functions in one file. This would allow all the function access to certain functions that the user should not see.

The functions that the user should not see are:

• stack_resize(struct stack *, size_t)
• stack_diagnose(const char *type, const char *diagnostic)
• stack_error(const char *str)
• stack_warn(const char *str)

The above functions should be defined as static functions within the module so that they do not impact the global name space.

It might be better if the function void stack_push(void *val, size_t size, struct stack *stack) called stack_resize(struct stack *, size_t) rather than forcing the user to resize the stack.

Note: none of the functions in stack_diagnostics.c are declared as externs where they are used or in a header file so that they are declared at run time; this could possibly cause some compilers to complain.

Possible Problems in the Code
I see two possible problems in the following code:

    if(!newsize)
        stack_error("resizing stack to size 0");
    else if(newsize < 16)
        stack_warn("resizing stack to size less than 16");

The first is that while the allocation error is reported, it is not handled. If stack_resize is called internally then there really should be error handling. If stack_resize is called explicitly by the user then the function should return at this point so that the user can handle the allocation error.

The second problem I see is that while in the rest of the function the if and else clauses are wrapped in braces this code is not wrapped in braces. First this is inconsistent and second for maintenance reasons it would be better if all if and else clauses were compound statements to allow for expansion of the code.

Include Files
Some compilers can't find the include file stack.h which is local to the code when it is used as

#include <stack.h>

While that usage would be correct if the file was in a library folder somewhere, it is not correct within the source code for the library itself unless a -I flag is set during the build.

It's not really clear what stack_internal.h is for.

The usage example is unrealistic, as it fails to show the error handling that's necessary when initialising the stack. Wrapping the initialisation in command-like macros makes it harder to write the correct checks (as we can't just use a return value, we have to inspect the macro to see where the result went):

STACK_INIT(8);
if (!stack_global) { /* not obviously connected to above */
    fprintf("Stack allocation failure\n");
    return EXIT_FAILURE;
}
STACK_FINI();

I'm not a big fan of expanding STACK_FINI() here:

#define STACK_INIT(size) \
do { \
    STACK_FINI(); \
    stack_global = stack_create(size); \
} while(0)

Firstly, I'd prefer the stack_global definition to explicitly initialize with a null pointer, rather than relying on the implicit initialization; secondly, I consider an init of an already inited stack to be a programming error - and we're already willing to abort() on underflow.

I'm concerned about these allocations in stack_create():

    if((ret = malloc(sizeof(*ret))))
    {
        ret->beg = malloc(size);
        ret->end = ret->cur = ret->beg + size;
    }
    return ret;

On the style side, I'd separate the assignment from the test, and give the variable a more meaningful name:

    stack = malloc(sizeof *stack);
    if (!stack) {
        return stack;
    }

More worryingly, what happens when this first allocation succeeds, but the allocation of size chars (for stack->beg) fails? We return a stack that's unusable, and callers need to test beg as well:

STACK_INIT(8);
if (!stack_global || !stack_global->beg) { /* even less obvious */
    fprintf("Stack allocation failure\n");
    return EXIT_FAILURE;
}
STACK_FINI();

A better approach, if beg can't be allocated, is to release the stack and return null:

    stack = malloc(sizeof *stack);
    if (!stack) {
        return stack;
    }
    stack->beg = malloc(size);
    if (!stack->beg) {
        free(stack);
        return NULL;
    }
    ret->end = ret->cur = ret->beg + size;

Other error handling is also suspect. For example, if the realloc() fails in stack_resize(), most programmers would expect the stack to be unchanged. But we have this code instead:

    newptr = realloc(stack->beg, newsize);

    if(!newptr)
    {
        free(stack->beg);
        return -1;
    }

So if we couldn't realloc, we now have a broken, unusable stack; any attempt to use it will dereference the dangling pointer, which is Undefined Behaviour. And in the success case, we fail to free the memory which is about to become inaccessible (a leak). I think what's intended is:

    newptr = realloc(stack->beg, newsize);
    if (!newptr) {
        return -1;
    }
    free(stack->beg);
    stack->beg = newptr;

Problems with the interface:

• There's no size() function to determine whether pop() and peek() can be called - the programmer needs to maintain their own count of elements.
• pop() aborts on underflow, but peek() does no checking, which is inconsistent.
• peek() requires a pointer to mutable stack, but would be expected to work with a const one.
• The term "safe" is highly misleading. If objects of differing sizes are pushed, the pop() calls must exactly match, as there's no checking built into the stack to record object sizes.

One issue that I see is that you will not be able to call functions in this library from C++.

To be able to do so, you need to wrap stack.h in an extern "C" block:

#ifdef __cplusplus
extern "C" {
#endif
/* functions */
#ifdef __cplusplus
}
#endif

    if((ret = malloc(sizeof(*ret))))
    {
        ret->beg = malloc(size);
        ret->end = ret->cur = ret->beg + size;
    }

You don't check the second malloc's returned value. When out of memory, you're likely to return an object in invalid state (or maybe not, since ret->end and ret->cur initialization is an UB anyway).