2
\$\begingroup\$

Last week I posted a previous version of this code. I improved the program and worked on it for a few days and I would love to get reviews, feedback and tips so I could learn more and more from you guys.
So here is the program:

config_man.h

#ifndef CONFIG_MAN_H
#define CONFIG_MAN_H

int check_file_existence(const char *);
int create_config_file(const char *, const char*);
int write_config_unit(const char *, const char *, const char *);
char *read_config_unit(const char *, const char *);
int check_unit_existence(const char *, char *);
int read_reg_syntax(char *, char *, int);
int read_list_syntax(char *, char *, int);

#endif

config_man.c

/*
*This header contains all required functions to manage configuration files.
*/ 
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "config_man.h"

/*
*The function will check if the given file in the 
*file path exists. It retruns 1 for yes and 0 for no.
*/
int check_file_existence(const char *file_path) {
    FILE *fp;

    fp = fopen(file_path, "r");
    if(fp == NULL) { //if file doesnt exists 
        fprintf(stderr, "Error occured due to missing configurations file.\n");
        return 0;
    }
    fclose(fp);
    return 1;
}

/*
*A function to create a configuration file with the given path.
*Then it'll write a comentted discription and instruction for this file.
*/ 
int create_config_file(const char *file_path, const char *description) {
    FILE *fp;
    const char *config_instruct = {
        "################################################################\n"
        "#--------------------------------------------------------------#\n"
        "#                        [Instructions]                        #\n"
        "#--------------------------------------------------------------#\n"
        "################################################################\n"
        "# Please it's very important to make sure there are 3 charact- #\n"
        "# ers between the unit name and its configurations.            #\n"
        "# For example:                                                 #\n"
        "#                                                              #\n"
        "# UnitName = configurations                                    #\n"
        "#                                                              #\n"
        "# As you can see there are 3 characters between the unit name  #\n"
        "# and the configurations (2 spaces and an equal sign). Be awa- #\n"
        "# re that each new line terminates the unit's configurations.  #\n"
        "# If the unit's configurations are too long you can put it in- #\n"
        "# side a pair of braces and make it a list. For example:       #\n"
        "#                                                              #\n"
        "# VeryLongUnit = {                                             #\n"
        "#    config_1                                                  #\n"
        "#    config_2                                                  #\n"
        "#    config_3                                                  #\n"
        "#    config_4                                                  #\n"
        "# }  <- Don't forget to close the list                         #\n"
        "#                                                              #\n"
        "# Be aware that '}' terminates the list.                       #\n"
        "# Note: Please if you want to use indention for the list, only #\n"
        "# use tabs, becuase they are ignored while reading a list.     #\n"
        "# It's also good to note that in case the unit is a commands   #\n"
        "# unit and this command requires root privileges, you can just #\n"
        "# prepend 'sudo' to it like this:                              #\n"
        "#                                                              #\n"
        "# CommandsUnit = sudo root_command                             #\n"
        "#                                                              #\n"
        "# Lastly as you can see hashes ('#') are ignored.              #\n"
        "# By the way, sorry for the hard syntax I really tried to make #\n"
        "# as easy as I can and that's the result. I hope you like it :)#\n"
        "################################################################\n"
    };

    fp = fopen(file_path, "w");
    if(fp == NULL) { //Check if error occurred
        fprintf(stderr, "An error occurred while creating: %s.\n", file_path);
        return -1;
    }
    fprintf(fp, "%s\n", description);
    fprintf(fp, "%s\n", config_instruct);
    fclose(fp);
    return 0;
}

/*
*This function takes a unit name and a description then it'll 
*write the description to it so youll know how to configure it.
*/
int write_config_unit(const char *file_path, const char *unit_name, const char *unit_desc) {
    FILE *fp;

    fp = fopen(file_path, "a");
    if(fp == NULL) {//Check if error occurred
        fprintf(stderr, "An error occurred while writing to: %s\n", file_path);
        return -1;
    }
    fprintf(fp, "%s = %s\n\n", unit_name, unit_desc);
    fclose(fp);
    return 0;   
}

/*
*This function will read all the config file, it'll
*ignore every commented line (with '#'), it will search 
*for the desired unit's confgis. If the unit exists it will 
*return a pointer to it, otherwise it will return NULL.
*/
char *read_config_unit(const char *file_path, const char *unit_name) {
    FILE *fp;
    char unit_buffer[600]; //Buffer for the unit name and its configs 
    char *configs_beginning; //Pointer to the targeted unit's configs beginning
    char *read_configs; //Array for the read configs
    unsigned int config_status = 0; //1 = list, 0 = one line configurations
    int status;

    fp = fopen(file_path, "r");
    if(fp == NULL) { //Check if error occurred
        fprintf(stderr, "An error occurred reading: %s\n", file_path);
        return NULL;
    }
    read_configs = (char *) calloc(801, sizeof(char));
    if(read_configs==NULL) {
        fprintf(stderr, "An error occurred while allocating memory.\n");
        return NULL;
    }
    //Loop in the file's content 
    while(fgets(unit_buffer, sizeof(unit_buffer), fp) != NULL) { 
        //If the beginning of a line is commented or empty
        if(*unit_buffer=='#' || *unit_buffer=='\n' || *unit_buffer=='\0') 
            continue; //Ignore and read next line
        if(config_status) { //If list was found
            if((status=read_list_syntax(unit_buffer, read_configs, 801)) == -1) {
                fprintf(stderr, "An overflow was detected while reading unit: %s.\n", unit_name);
                fclose(fp);
                free(read_configs);
                return NULL;
            }
            else if(status) //If reading list
                continue; //read the next line of the list
            fclose(fp);
            return read_configs;
        }
        else {
            status=check_unit_existence(unit_name, unit_buffer);
            
            if(status)  //If unit wasnt found in line
                continue; //Read the next one 
            /*The address of the beginning of the unit's configurations =
            beginning of the line + len of the unit name + 3 bytes (2 spaces and '=' sign)*/
            configs_beginning = unit_buffer + strlen(unit_name) + 3;
            if(*(configs_beginning) == '{') { //If unit configs is beginning of a list
                config_status = 1;
                continue; 
            }
            else {
                if(read_reg_syntax(configs_beginning, read_configs, 801) == -1) { 
                    fprintf(stderr, "An overflow was detected while reading unit: %s.\n", unit_name);
                    fclose(fp);
                    free(read_configs);
                    return NULL;
                }
                fclose(fp);
                return read_configs;
            }
        }
    }

    fclose(fp);
    free(read_configs);
    fprintf(stderr, "Error occured while searching for unit: %s.\n", unit_name);
    return NULL;
}

/*
*This function reads the regular configurations syntax,
*Then passes it to the config_buffer. If there was a buffer 
*overflow the function will return -1, otherwise 0.
*/
int read_reg_syntax(char *config_beginning, char *configs_buffer, int buffer_size) {
    int i = 0;

    while(1) 
        switch(config_beginning[i]) {
            case '\n':
                if(i+1 > buffer_size) 
                    return -1;
                configs_buffer[i] = '\0'; //teminate line
                return 0; //exit
            case '\0': //Line is terminated
                return 0; //exit
            default:
                //Check for overflow
                if(i+1 > buffer_size) 
                    return -1;
                //insert char into the configs_buffer
                configs_buffer[i] = config_beginning[i];
                i++;
                break;
            }
}

/*
*This function will read the syntax of a list, if finished reading itll 
*return 0, if still reading it will return 1 to read the next line, if 
*overflow occured -1 is returned.
*/
int read_list_syntax(char *line_beginning, char *configs_buffer, int buffer_size) {
    int char_cnt, i;

    char_cnt = strlen(configs_buffer); //number of characters in configs_buffer
    i = 0;  
    while(1) { //Loop and read line until reaching '\n' or ';'
        switch(line_beginning[i]) {
            case '}': //If end of list
                configs_buffer[char_cnt] = '\0'; //terminate line
                return 0; //Finished reading list
            case '\t': //Ignore indention 
                break;
            case '\n':
                //Make sure there is no overflow
                if(char_cnt+1 > buffer_size)
                    return -1;
                configs_buffer[char_cnt] = line_beginning[i];
                return 1; //Read next line
            default:
                //Make sure there is no overflow
                if(char_cnt+1 > buffer_size)
                    return -1;
                configs_buffer[char_cnt] = line_beginning[i];
                char_cnt++;
                break;
        }
        i++;
    }
}

/*
*This function takes a unit name and pointer to the beginning
*of the unit's line and it then it finds the unit's name which is
*the first word then it checks if the founded unit and the passed 
*one are equal and the same.
*0 = equal, 1 = not equal, -1 error.
*/
int check_unit_existence(const char *unit_name, char *line_begin) {
    unsigned int i;
    int unit_name_len = strlen(unit_name);
    char unit_to_check[unit_name_len]; //Array for the unit name that needs to be checked

    for(i=0; i<unit_name_len+1; i++) {
        if(line_begin[i]==' ' || line_begin[i]=='\t' || line_begin[i]=='\n') {
            unit_to_check[i] = '\0';
            break;
        }
        unit_to_check[i] = line_begin[i];
    }
    if(strcmp(unit_name, unit_to_check) == 0)
        return 0;
    else //Not equal
        return 1;
}

sys_backup.h

#ifndef SYS_BACKUP_H
#define SYS_BACKUP_H

typedef struct {
    int day;
    int month;
    int year;
} date;

int rm_dir(const char *);
void get_date(date *);
char *make_backup_dir(const char *, date);
char *split_configs(char *, unsigned int *);
int exec_command(const char *, char **);
char *get_name(const char *);
void clean_line(char *);

#endif

sys_backup.c

#include <stdio.h>
#include <time.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <dirent.h>
#include <errno.h>
#include "sys_backup.h"
/*
*Recursive function to delete directories.
*Returns -1 for failure, 1 if cant open dir, otherwise 0.
*/
int rm_dir(const char *dir_path) {
    DIR *dr;
    struct dirent *dp;
    struct stat statbuf;
    unsigned int original_path_len, new_path_len;
    char *new_path;

    original_path_len = strlen(dir_path);
    if((dr=opendir(dir_path)) == NULL) { //If failed opening dir
        if(errno==ENOENT) { //If dir doesnt exists
            fprintf(stderr, "Directory doesn't exists: %s\n", dir_path);
            return 1;
        }
        else {
            fprintf(stderr, "Error occured while opening directory: %s\n", dir_path);
            return -1;
        }
    }

    //Loop in dir's content
    while((dp=readdir(dr))!=NULL) {
        //Ignore '.' and '..' directories
        if (strcmp(dp->d_name, ".")==0 || strcmp(dp->d_name, "..")==0)
            continue;
        new_path_len = original_path_len + strlen(dp->d_name) + 2; //2 = '\0' and '/'
        new_path = (char *) malloc(new_path_len);
        if(new_path == NULL) {
            closedir(dr);
            return -1;
        }
        sprintf(new_path, "%s/%s", dir_path, dp->d_name);
        //Get object status
        if(stat(new_path, &statbuf)==0) {
            if(S_ISDIR(statbuf.st_mode)) { //If object is a directory 
                if(rm_dir(new_path)==-1) { //Delete it 
                    free(new_path);
                    closedir(dr);
                    fprintf(stderr, "Error occured while removing directory: %s\n", new_path);
                    return -1;
                }
            }
            else {
                if(unlink(new_path)==-1) { //Delte file
                    free(new_path);
                    closedir(dr);
                    fprintf(stderr, "Error occured while removing file: %s\n", new_path);
                    return -1;
                }
            }
            free(new_path);
        }
        else {
            free(new_path);
            closedir(dr);
            fprintf(stderr, "An unknown error occured.\n");
            return -1;
        }
    }
    closedir(dr);
    rmdir(dir_path);
    return 0;
}

/*
*This function will get the date of today, and it'll insert it to the passed date array.
*/
void get_date(date *date_struct) {
    long int sec_since_epoch;
    struct tm current_time, *time_ptr;

    sec_since_epoch = time(0); 
    time_ptr = &current_time; //Set time pointer to the current_time struct
    localtime_r(&sec_since_epoch, time_ptr);

    //Pass today's date to the date struct  
    date_struct->day = time_ptr->tm_mday;
    date_struct->month = time_ptr->tm_mon + 1; //+1 because months range from 0 - 11
    date_struct->year =  time_ptr->tm_year - 100; //-100 because tm_year is number of passed years since 1900
}

/*
*A function that gets pointer to int array that contains the
*date of today and create a backup dir in the passed path 
*passed date. Then it will return the full path of the created dir. 
*/
char *make_backup_dir(const char *device_path, date date_struct) {
    char dir_name[9];
    char *backup_path; 

    //Convert the date_array to a string so will use it to name the dir in the device path
    sprintf(dir_name, "%02d-%02d-%02d", date_struct.day, date_struct.month, date_struct.year);
    //Prepare the full backup path 
    backup_path = (char *) malloc(sizeof(dir_name)+strlen(device_path)+1);
    if(backup_path == NULL) {
        fprintf(stderr, "Error occured while allocating memory.\n");
        return NULL;
    }
    sprintf(backup_path, "%s%s/", device_path, dir_name); 

    if(mkdir(backup_path, S_IRWXU)==-1) { //If failed
        fprintf(stderr, "Error occured while creating directory: %s\n", backup_path);
        free(backup_path); 
        return NULL;
    }
    printf("Created: %s\n", backup_path);
    return backup_path;
}

/*
*This function is going to split a read list of configurations
*into a separated lines, then return a pointer to it. NULL for 
*failure.
*/
char *split_configs(char *configs, unsigned int *i_in_configs) {
    static char line[600];
    unsigned int i;

    if(configs[*i_in_configs]=='\0') //If the beginning of configs is terminated
        return NULL;

    memset(line, '\0', sizeof(line)); //Make sure the static array is empty
    for(i=0; configs[*i_in_configs]!='\0'; i++) { 
        if(configs[*i_in_configs]=='\n') {
            *i_in_configs = *i_in_configs + 1; //Skip it and break
            break;
        }
        line[i] = configs[*i_in_configs];
        *i_in_configs = *i_in_configs + 1;
    }
    return line;
} 

/*
*This function will fork the parent process to create a
*child process then execute commands using one of the exec
*familie's functions. Return -1 for fork failure, 1 for execv() 
*failure, otherwise 0.
*/
int exec_command(const char *prog_name, char *commands[]) {
    char prog_path[strlen("/usr/bin/")+strlen(prog_name)];
    int status;
    pid_t pid;
    pid_t ret;

    sprintf(prog_path, "/usr/bin/%s", prog_name);
    pid = fork(); //Create a new child process
    if(pid == -1) {//If failed to create new child
        fprintf(stderr, "An error occured while creating a child process.\n");
        return -1;
    }
    else if(pid != 0) { //If child process didnt start
        while((ret = waitpid(pid, &status, 0)) == -1) { //Wait for child
            if(errno != EINTR) { //If the waitpid() error isnt an interrupte signal
                fprintf(stderr, "An error occured while waiting for the child process.\n");
                return -1;
            }
        }
    }
    else 
        if(execv(prog_path, commands)==-1) //If command wasnt found
            return 1;
    return 0; 
}

/*
*This function gets a pointer to a line and then it 
*gets the first word in yhr line and return a pointer to it.
*/
char *get_name(const char *line) {
    static char name[200];
    unsigned int i; 

    for(i=0; i<sizeof(name); i++) {
        if(line[i]==' ' || line[i]=='\n' || line[i]=='\t') {
            name[i] = '\0';
            break;
        }
        name[i] = line[i];
    }
    if(*name=='\0') //If name is empty
        return NULL;
    return name;
} 

/*
*This function is going to check if the line ends with any space, 
*and modify it if it is since the exec_command and make_backup_dir()
*functions are space sensitive.
*/
void clean_line(char *line) {
    unsigned int i = 1;
    int len = strlen(line);

    while(line[len-i] == ' ') { //Clean line from spaces at the end
        line[len-i] = '\0';
        i++;
    }
}

main.c

/*
*This program will make some cleaning that you regularly do 
*before the full system backup. And then itll create new dir 
*in your storage device with date, to make the system backup in
*it useng rsync. The program will use system() to connect all the
*command line tools together and automate this process. Lastly 
*its good to note that tho program reads all the commands and 
*your customaized cleaning process from a config file with the 
*following path: ~/.config/sys_backup
*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include "config_man.h" //For config files managment 
#include "sys_backup.h"

int main(int argc, char *argv[]) {
    const char *config_desc = {
        "################################################################\n"
        "# This configuration file purposes are to provide to the       #\n"
        "# sys_backup.c program the right cleaning process and storage  #\n"
        "# device path, customized to your own needs and preferences.   #\n"
        "################################################################"
    };
    const char *units_list[] = {
        "DirsToClean", "CleaningCommands", "DevicePath", 
        "RsyncOpt", "DirsToBackup", 
        "\0"
    };
    const char *units_desc[] = {
        "Dirs path you regularly clean, like some cache dirs",
        "{\n"
        "\tCommands for cleaning your sysem, like:\n"
        "\tsudo pacman -Sc (for deleting uninstalled packages\n"
        "\tfrom the cache in arch based distros)\n"
        "}", 
        "Yous storage device's path",
        "Rsync backup option, for example: -aAXHv",
        "Directories to backup", 
        "\0"
    };
    char *config_path = ".config/sys_backup";
    char *home = getenv("HOME");
    char config_full_path[strlen(home)+strlen(config_path)+1];
    char *configurations, *backup_path, *ptr, *prog_name;
    char *command_arg, *commands_list[7];
    char new_prog_name[200+strlen("/usr/bin/")];
    int opt, status;
    unsigned int i, len;
    date dir_name;

    /*Prepare configurations file's full path*/
    sprintf(config_full_path, "%s/%s", home, config_path);

    while((opt = getopt(argc, argv, ":c")) != EOF) {
        if(opt == 'c' && argc<3) {
            if(create_config_file(config_full_path, config_desc)==-1) 
                exit(EXIT_FAILURE);
            for(i=0; *units_list[i]!='\0'; i++)
                if(write_config_unit(config_full_path, units_list[i], units_desc[i])==-1) 
                    exit(EXIT_FAILURE);
            printf("%s was succesfully generated.\n", config_full_path);
            return 0;
        }
        else if(argc > 3) {
            fprintf(stderr, "Too many arguments were given.\n");
            exit(EXIT_FAILURE);
        }
        else {
            fprintf(stderr, "The argument %s is invalid.\n", argv[1]);
            exit(EXIT_FAILURE);
        }
    }
    if(check_file_existence(config_full_path) == 0) { //Check if config file exists
        fprintf(stderr, "Reminder: you can always generate new one using the -c option.\n");
        exit(EXIT_FAILURE);
    }
    /*Read the configured units*/
    //DirsToClean
    if((configurations=read_config_unit(config_full_path, units_list[0])) == NULL) //Unit wasnt found
        exit(EXIT_FAILURE);
    /*Separate each dir path on its own to delete it*/
    i = 0;
    while((ptr=split_configs(configurations, &i)) != NULL) {
        if(*ptr=='/' && ptr[1]=='\0'){ //If path is stand alone root tree
            fprintf(stderr, "Error: you can't remove root tree.\n");
            fprintf(stderr, "Please make sure that there is no sign of stand alone '/' in your configs.\n");
            free(configurations);
            exit(EXIT_FAILURE);
        }
        clean_line(ptr);
        if((status=rm_dir(ptr)) == -1) {
            free(configurations);
            exit(EXIT_FAILURE);
        }
        else if(status) //If cant open dir skip it 
            ;
        else
            printf("Removing: %s\n", ptr);
    }
    free(configurations);

    //CleaningCommands
    if((configurations=read_config_unit(config_full_path, units_list[1])) == NULL) //Unit wasnt found
        exit(EXIT_FAILURE);
    /*Separate each cleaning command on its own to execute it*/
    i = 0;
    memset(commands_list, '\0', sizeof(commands_list)); //Make sure array is empty
    while((ptr=split_configs(configurations, &i)) != NULL) {
        if((prog_name=get_name(ptr))==NULL) { //If no program name was found
            fprintf(stderr, "An unknown error occured\n");
            free(configurations);
            exit(EXIT_FAILURE);
        }
        /*Prepare command*/
        if(strcmp(prog_name, "sudo") == 0) { //If command starts with sudo
            commands_list[0] = "sudo";
            prog_name = get_name(ptr+strlen("sudo ")); //Get the name after sudo
            sprintf(new_prog_name, "/usr/bin/%s", prog_name); 
            commands_list[1] = new_prog_name; //Insert it with the full path
            command_arg = ptr+strlen("sudo ")+strlen(prog_name) + 1; //skip all the read characters
            clean_line(command_arg);
            commands_list[2] = command_arg; 
            printf("Warning: Executing the command '%s' with root privileges!\n", ptr);
            status = exec_command(commands_list[0], commands_list);
            if(status==-1) {
                free(configurations);
                exit(EXIT_FAILURE);
            }
            if(status)
                _Exit(127);
        } 
        else {
            command_arg = ptr+strlen(prog_name)+1; //+1 to ommit the space
            clean_line(command_arg);
            commands_list[0] = prog_name;
            commands_list[1] = command_arg;
            status = exec_command(commands_list[0], commands_list);
            if(status==-1) {
                free(configurations);
                exit(EXIT_FAILURE);
            }
            if(status)
                _Exit(127);
        }
    }
    free(configurations);

    //DevicePath
    if((configurations=read_config_unit(config_full_path, units_list[2])) == NULL) //Unit wasnt found
        exit(EXIT_FAILURE);
    get_date(&dir_name); //Get the date of today and pass it to dir_name
    if((backup_path=make_backup_dir(configurations, dir_name))==NULL) { //Create backup dir and get its path
        free(configurations);
        exit(EXIT_FAILURE);
    }
    free(configurations);

    /*Prepare Rsync commands*/
    memset(commands_list, '\0', sizeof(commands_list)); //Make sure array is empty
    //RsyncOpt
    if((configurations=read_config_unit(config_full_path, units_list[3])) == NULL) {
        free(backup_path);
        exit(EXIT_FAILURE);
    }
    clean_line(configurations);

    commands_list[0] = "sudo";
    commands_list[1] = "/usr/bin/rsync";
    commands_list[2] = configurations;
    commands_list[4] = backup_path;

    //DirsToBackup
    if((configurations=read_config_unit(config_full_path, units_list[4])) == NULL) {
        free(commands_list[2]);
        free(commands_list[4]);
        exit(EXIT_FAILURE);
    }

    len = 0;
    while(1) {
        ptr = get_name(configurations+len);
        if(ptr != NULL) {
            commands_list[3] = ptr;

            status = exec_command(commands_list[0], commands_list);
            if(status==-1) {
                free(commands_list[2]);
                free(configurations);
                free(commands_list[4]);
                exit(EXIT_FAILURE);
            }
            if(status)
                _Exit(127);
            len += strlen(ptr) + 1; //to ommit the already checked names 
        }
        else {
            free(commands_list[2]);
            free(configurations);
            free(commands_list[4]);
            break;
        }
    }
    return 0;
}

Here is a link to the repo for the ones who would prefer to review it there

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Missing const

I see you are using const in a lot of places, but you have missed a few:

  • check_unit_existence(): both arguments can be made const char *
  • read_reg_syntax(): the first arugment can be made const char *
  • read_list_syntax(): same as read_reg_syntax()
  • split_configs(): the first argument can be made const char *
  • exec_command(): the second argument can be made a const char **

There is also config_path and home in main() which should be made const.

Use size_t for sizes and counts

Your compiler should have warned you about comparisons between signed and unsigned integers. If not, enable warnings and try to fix them all! The result of strlen() and sizeof has type size_t, which is an unsigned integer that is probably 64 bits on your machine, and its value can be larger than what can be held in an int. So to avoid problems, use size_t.

Checking for the existence of a file

There are several issues with your check for the existence of a file. The first is that fopen() can fail for more reasons than just the file not existing: it could exist but there can be a read error, or it can exist but you don't heave read permissions. But more importantly, there is no guarantee that the file still exists after this function returns, so you cannot depend on the result, otherwise you might have created a TOCTTOU bug.

The check is completely redundant anyway, since in read_config_unit(), you correctly check the return value of fopen() again. So just remove the check_file_existence() function.

Also check for errors occuring after opening a file

A file might be opened succesfully, but an error might occur while you are reading or writing data. So it's not enough to check the return value of fopen(), you should also check the return values of fprintf(), fgets() and even fclose(). However, you don't have to do this for every I/O function; errors during file I/O are remembered and can be checked with ferror(). Make sure you report any of these errors, and exit the program with a non-zero exit code.

Don't hardcode magic numbers

Why is unit_buffer 600 bytes? Why is read_configs 801? Are you sure you used 801 everywhere correctly, and did not accidentily write 800? Instead of writing these so-called magic numbers directly in your code, create named constants for them. This avoids mistakes and makes it easy to change the value in a single place.

Prefer for-loops where appropriate

I see while(1) loops in a few places, and this is a code smell. For example, in read_reg_syntax(), you are iterating over the string config_beginning. The idiomatic way to do this in C is to use a for-loop, like so:

for (size_t i = 0; i < buffer_size && config_beginnings[i]; i++) {
    switch(config_beginning[i]) {
        case '\n':
            configs_buffer[i] = '\0';
            return;
        default:
            configs_buffer[i] = config_beginning[i];
            break;
     }
}

The big advantage is that all the information how you iterate over the string is now in one place: you start at the beginning, you go on until you either hit the end of the buffer or the end of the string, and you simply advance the iterator by one each time.

Note that if you read config_beginning with fgets(), the buffer is guaranteed to have a NUL-byte in it, so you don't actually need the buffer size.

Use isspace() to check for whitespace characters

If you want to check if a character is whitespace, you can use isspace() to check for that.

Avoid using sprintf()

Since sprintf() doesn't check for the size of the buffer it writes to, it is very easy to make a mistake in the code and allow for a buffer overflow to occur. It looks like you did not make this mistake, but this is such a common problem that you should just use snprintf() everywhere, even if you think you know for certain that the buffer is large enough.

Avoid returning pointers to local variables

The function split_configs() returns a pointer to the array line. You made this a static variable, so the array will still exist after the function returns, but this is a bit of a dubious practice, since it means the function is not reentrant. It is better to either return a dynamically allocated buffer, or have the caller pass a pointer and a size for a buffer to this function, and let the caller decide how to allocate the buffer.

Possible read past end of buffer

What if prog_name is "sudo" but that was the whole command? Then when you write ptr + strlen("sudo "), you are reading past the end of the string.

\$\endgroup\$
1
  • \$\begingroup\$ Hi @G. Sliepen. First of all thanks for your time, your detailed review and all your tips you gave me. I really appreciate it and you taught me a lot and really thats my whole point for posting here -to learn from such amazing people like you-. Second I will of course listen to all of your tips, I will do all the necessary research and I will gladly modify the code and improve it. Any way thanks and I wish you the best. \$\endgroup\$
    – yan_kh
    Commented Mar 25, 2021 at 6:17

Your Answer

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

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