CLI Generator & Parser

Question

I have written a module that allows the developer to generate a CLI and parse given arguments. I am fairly new to C and would love to hear feedback on my code to improve it. All feedback is welcome however I am mainly concerned about best practises and optimisation. Included is the header file and the C file.

// cli.h

#ifndef CLI_H
#define CLI_H

#include <stdbool.h>

typedef enum
{
    NODE,
    GIT,
    C_FILE,
} CLIArgumentType;

typedef struct
{
    char* short_hand;
    char* long_hand;
    bool has_value;
    CLIArgumentType type;
} CLIArgumentTemplate;

typedef struct 
{
    CLIArgumentType type;
    char* value;
} CLIArgument;

extern CLIArgumentTemplate* cli_args_template;
extern CLIArgument* cli_args;

// Gets arguments & initialises variables where required
void cli_init(int argc, char** argv);

// Add a cli argument template which is used by the parser
void cli_arg_template_add(char* short_hand, char* long_hand, bool has_value, CLIArgumentType type);

// Parses the arguments provided in argv and adds them to cli_args
bool cli_args_parse();

// Returns an argument if exists
CLIArgument* cli_get_arg(CLIArgumentType type);

#endif // CLI_H

// cli.c

#include "cli.h"
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <stdio.h>

CLIArgumentTemplate* cli_args_template;
int args_template_len = 0;

CLIArgument* cli_args;
int args_len = 0;

int argc;
char** argv;

void cli_init(int arg_count, char** arg_vec)
{
    argc = arg_count;
    argv = arg_vec;

    cli_args_template = (CLIArgumentTemplate*)malloc(sizeof(CLIArgumentTemplate));
    cli_args = (CLIArgument*)malloc(sizeof(CLIArgument));

    // Remove directory argument
    argv++;
    argc--;
}

void cli_arg_template_add(char* short_hand, char* long_hand, bool has_value, CLIArgumentType type)
{
    CLIArgumentTemplate* arg_template = (CLIArgumentTemplate*)malloc(sizeof(CLIArgumentTemplate));
    arg_template->short_hand = short_hand;
    arg_template->long_hand = long_hand;
    arg_template->has_value = has_value;
    arg_template->type = type;

    cli_args_template = realloc(cli_args_template, sizeof(CLIArgumentTemplate) * ++args_template_len);
    cli_args_template[args_template_len - 1] = *arg_template;
}

bool cli_args_parse()
{
    int arg_index = 0;
    char* current_argument;

    while (arg_index < argc) {
        current_argument = argv[arg_index];

        char first_two_chars[2];
        strncpy(first_two_chars, current_argument, 2);
        if (current_argument[0] != '-' && (strcmp(first_two_chars, "--") != 0)) {
            printf("[PARSING ERROR]: Invalid argument '%s'. Argument must begin with dashses.", current_argument);
            return false;
        }

        CLIArgument* argument = NULL;

        for (int i = 0; i < args_template_len; i++) {
            CLIArgumentTemplate template = cli_args_template[i];
            char* argument_stripped = current_argument;
            if (strcmp(++argument_stripped, template.short_hand) != 0 && strcmp(++argument_stripped, template.long_hand) != 0) continue;

            argument = (CLIArgument*)malloc(sizeof(CLIArgument));
            argument->type = template.type;
            
            if (template.has_value) {
                if (++arg_index >= argc) {
                    printf("[PARSING ERROR]: Could not find value for argument '%s'.", current_argument);
                    return false;
                }

                char* next_word = argv[++arg_index];
                argument->value = next_word;
            }

            break;
        }

        if (argument == NULL) {
            printf("[PARSING ERROR]: Unrecognised argument '%s'.", current_argument);
            return false;
        }

        cli_args = realloc(cli_args, sizeof(CLIArgument) * ++args_len);
        cli_args[args_len - 1] = *argument;

        arg_index++;     
    }

    return true;
}

CLIArgument* cli_get_arg(CLIArgumentType type)
{
    for (int i = 0; i < args_len; i++) {
        if (cli_args[i].type == type) {
            return &cli_args[i];
        }
    }

    return NULL;
}

Answer 1

This code is dangerous:

CLIArgumentTemplate* arg_template = (CLIArgumentTemplate*)malloc(sizeof(CLIArgumentTemplate));
arg_template->short_hand = short_hand;
arg_template->long_hand = long_hand;
arg_template->has_value = has_value;
arg_template->type = type;

If malloc() returns a null pointer, then we have Undefined Behaviour, because we attempt to dereference it when accessing the members. We need to test for that case, and respond appropriately if arg_template is null.

Also, malloc() returns a void*, which can be assigned to any pointer type without a cast, and it's good practice to avoid repeating the type name. I suggest:

CLIArgumentTemplate *arg_template = malloc(sizeof *arg_template);
if (!arg_template) {
    return false;
}
arg_template->short_hand = short_hand;
arg_template->long_hand = long_hand;
arg_template->has_value = has_value;
arg_template->type = type;

Even more dangerous is

cli_args_template = realloc(cli_args_template, sizeof(CLIArgumentTemplate) * ++args_template_len);

In this case, a failure, even if caught like the above, will result in a memory leak because we overwrite cli_args_template, which leaves no way to access the memory it previously pointed to. We need something like

 void *new_mem = realloc(cli_args_template,
                         sizeof *cli_args_template * (args_template_len + 1));
 if (!new_mem) {
     free(arg_template);
     return false;
 }
 cli_args_template = new_mem;
 ++args_template_len;

Actually, reading cli_arg_template_add() completely, it seems that we never release arg_template, and there's no good reason to allocate this object and copy it into cli_args_template. Just create it directly there:

bool cli_arg_template_add(char* short_hand,
                          char* long_hand,
                          bool has_value,
                          CLIArgumentType type)
{
    void *new_mem = realloc(cli_args_template,
                         sizeof *cli_args_template * (args_template_len + 1));
    if (!new_mem) {
        return false;
    }
    cli_args_template = new_mem;
    CLIArgumentTemplate* arg_template = cli_args_template + args_template_len++;
    arg_template->short_hand = short_hand;
    arg_template->long_hand = long_hand;
    arg_template->has_value = has_value;
    arg_template->type = type;
    return true;
}

You have other similar memory allocation problems elsewhere in the code that can be found with a half-decent static analyser or a runtime memory tool such as Valgrind memcheck.