[1/2] misc-progs: addonctrl: Add support for 'Services' metadata

  * Addonctrl will now check in addon metadata for the exact initscript
  names (Services). If more than one initscript is defined for an addon,
  the requested action will be performed on all listed initscripts.
* Added posibility to perform action on a specific initscript of an
  addon instead of on all initscripts of the addon.
* New action 'list-services' to display a list of services related to
  an addon.
* New action 'boot-status' to display wether service(s) are enabled
  to start on boot or not.
* More error checking and cleaner error reporting to user
* General cleanup and code restructuring to avoid code duplication
* Updated and made usage instructions more verbose.

Fixes: Bug#12935
Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
---
 src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++------
 1 file changed, 323 insertions(+), 61 deletions(-)
  

Tested-by: Adolf Belka <adolf.belka@ipfire.org>


On 03/10/2022 17:27, Robin Roevens wrote:
> * Addonctrl will now check in addon metadata for the exact initscript
>    names (Services). If more than one initscript is defined for an addon,
>    the requested action will be performed on all listed initscripts.
> * Added posibility to perform action on a specific initscript of an
>    addon instead of on all initscripts of the addon.
> * New action 'list-services' to display a list of services related to
>    an addon.
> * New action 'boot-status' to display wether service(s) are enabled
>    to start on boot or not.
> * More error checking and cleaner error reporting to user
> * General cleanup and code restructuring to avoid code duplication
> * Updated and made usage instructions more verbose.
>
> Fixes: Bug#12935
> Signed-off-by: Robin Roevens<robin.roevens@disroot.org>
> ---
>   src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++------
>   1 file changed, 323 insertions(+), 61 deletions(-)
>
> diff --git a/src/misc-progs/addonctrl.c b/src/misc-progs/addonctrl.c
> index 14b4b1325..277bd1809 100644
> --- a/src/misc-progs/addonctrl.c
> +++ b/src/misc-progs/addonctrl.c
> @@ -10,71 +10,333 @@
>   #include <string.h>
>   #include <unistd.h>
>   #include <sys/types.h>
> +#include <sys/stat.h>
>   #include <fcntl.h>
> +#include <dirent.h>
> +#include <errno.h>
>   #include "setuid.h"
>   
>   #define BUFFER_SIZE 1024
>   
> +const char *enabled_path = "/etc/rc.d/rc3.d";
> +const char *disabled_path = "/etc/rc.d/rc3.d/off";
> +
> +char errormsg[BUFFER_SIZE] = "";
> +char *usage =
> +    "Usage\n"
> +    "  addonctrl <addon> (start|stop|restart|reload|enable|disable|status|boot-status|list-services) [<service>]\n"
> +    "\n"
> +    "Options:\n"
> +    "  <addon>\t\tName of the addon to control\n"
> +    "  <service>\t\tSpecific service of the addon to control (optional)\n"
> +    "  \t\t\tBy default the requested action is performed on all related services. See also 'list-services'.\n"
> +    "  start\t\t\tStart service(s) of the addon\n"
> +    "  stop\t\t\tStop service(s) of the addon\n"
> +    "  restart\t\tRestart service(s) of the addon\n"
> +    "  enable\t\tEnable service(s) of the addon to start at boot\n"
> +    "  disable\t\tDisable service(s) of the addon to start at boot\n"
> +    "  status\t\tDisplay current state of addon service(s)\n"
> +    "  boot-status\t\tDisplay wether service(s) is enabled on boot or not\n"
> +    "  list-services\t\tDisplay a list of services related to the addon";
> +
> +// Check if <text> matches <pattern>. Wildcard '?' matches any single character.
> +// Returns 1 when found, 0 when not found
> +int match(const char *pattern, const char *text) {
> +    if (pattern[0] == '\0' && *text == '\0')
> +        return 1;
> +    if (*text != '\0' && (pattern[0]=='?' || pattern[0]==*text))
> +        return match(pattern+1, text+1);
> +    return 0;
> +}
> +
> +// Find a file <path> using <filepattern> allowing the '?' wildcard.
> +// Returns the found filename or NULL if not found
> +char* find_file_in_dir(const char *path, const char *filepattern)
> +{
> +    static struct dirent *entry;
> +    DIR *dp;
> +    int found = 0;
> +
> +    if ((dp = opendir(path)) != NULL) {
> +        while(found == 0 && (entry = readdir(dp)) != NULL)
> +            found = match(filepattern, entry->d_name);
> +
> +        closedir(dp);
> +    }
> +
> +    if (! found) {
> +        return NULL;
> +    }
> +
> +    return entry->d_name;
> +}
> +
> +// Reads Services metadata for <addon>.
> +// Returns pointer to array of strings containing the services for <addon>
> +// and sets <servicescnt> to the number of found services
> +char **get_addon_services(const char *addon, int *servicescnt) {
> +    const char *metafile_prefix = "/opt/pakfire/db/installed/meta-";
> +    const char *metadata_key = "Services";
> +    const char *keyvalue_delim = ":";
> +    const char *service_delim = " ";
> +    char *token;
> +    char **services = NULL;
> +    char *service;
> +    char line[BUFFER_SIZE];
> +    int i = 0;
> +    char *metafile = malloc((strlen(metafile_prefix) + strlen(addon) + 1) * sizeof(char));
> +
> +    sprintf(metafile, "%s%s", metafile_prefix, addon);
> +    FILE *fp = fopen(metafile,"r");
> +    if ( fp ) {
> +        // Get initscript(s) for addon from meta-file
> +        while (!feof(fp) && services == NULL) {
> +            if (fgets(line, BUFFER_SIZE - 1, fp) != NULL) {
> +                // Strip newline
> +                char *newline = strchr(line, '\n');
> +                if (newline) *newline = 0;
> +
> +                // Parse key/value and look for required key.
> +                token = strtok(line, keyvalue_delim);
> +                if (token != NULL && strcmp(token, metadata_key) == 0) {
> +                    token = strtok(NULL, keyvalue_delim);
> +                    if (token != NULL) {
> +                        services = malloc((strlen(token) + 1) * sizeof (char *));
> +
> +                        // Put each service in services array
> +                        service = strtok(token, service_delim);
> +                        while (service != NULL) {
> +                            services[i] = malloc((strlen(service) + 1) * sizeof (char));
> +                            strcpy(services[i++], service);
> +
> +                            service = strtok(NULL, service_delim);
> +                        }
> +                    }
> +                }
> +            } else {
> +                snprintf(errormsg, BUFFER_SIZE - 1, "Could not read '%s' metadata for addon '%s'.", metadata_key, addon);
> +            }
> +        }
> +        fclose(fp);
> +    }  else {
> +        snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not found.\n\n%s", addon, usage);
> +    }
> +
> +    free (metafile);
> +    *servicescnt = i;
> +    return services;
> +}
> +
> +// Calls initscript <service> with parameter <action>
> +int initscript_action(const char *service, const char *action) {
> +    const char *initd_path = "/etc/rc.d/init.d";
> +    char *command = malloc((strlen(initd_path) + 1 + strlen(service) + 1) * sizeof(char));
> +    int r = 0;
> +
> +    sprintf(command, "%s/%s %s", initd_path, service, action);
> +    r = safe_system(command);
> +    free(command);
> +
> +    return r;
> +}
> +
> +// Move an initscript with filepattern from <src_path> to <dest_path>
> +// Returns:
> +//   -1: Error during move. Details in errno (returned by C rename function)
> +//   0: Success
> +//   1: file was not moved, but is already in <dest_path>
> +//   2: file does not exist in either in <src_path> or <dest_path>
> +int move_initscript_by_pattern(const char *src_path, const char *dest_path, const char *filepattern) {
> +    char *src, *dest;
> +    int r = 1;
> +    char *filename = find_file_in_dir(src_path, filepattern);
> +
> +    if ( filename != NULL ) {
> +        src = malloc((strlen(src_path) + 1 + strlen(filename) + 1) * sizeof(char));
> +        dest = malloc((strlen(dest_path) + 1 + strlen(filename) + 1) * sizeof(char));
> +        sprintf(src, "%s/%s", src_path, filename);
> +        sprintf(dest, "%s/%s", dest_path, filename);
> +
> +        r = rename(src, dest);
> +
> +        free(src);
> +        free(dest);
> +    } else {
> +        filename = find_file_in_dir(dest_path, filepattern);
> +        if (filename == NULL)
> +            r = 2;
> +    }
> +
> +    return r;
> +}
> +
> +// Enable/Disable addon service(s) by moving initscript symlink from/to disabled_path
> +int toggle_service(const char *service, const char *action) {
> +    const char *src_path, *dest_path;
> +    char *filepattern = malloc((3 + strlen(service) + 1) * sizeof(char));
> +    int r = 0;
> +
> +    sprintf(filepattern, "S??%s", service);
> +
> +    if (strcmp(action, "enable") == 0) {
> +        src_path = disabled_path;
> +        dest_path = enabled_path;
> +    } else {
> +        src_path = enabled_path;
> +        dest_path = disabled_path;
> +    }
> +
> +    // Ensure disabled_path exists
> +    if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + S_IROTH + S_IXOTH) == -1 && errno != EEXIST) {
> +        r = 1;
> +        snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s. (Error: %d)", disabled_path, errno);
> +    } else {
> +        r = move_initscript_by_pattern(src_path, dest_path, filepattern);
> +        if (r == -1 ) {
> +            r = 1;
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. (Error: %d)", action, service, errno);
> +        } else if (r == 1) {
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is already %sd. Skipping...", service, action);
> +        } else if (r == 2) {
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s service %s. (Service has no valid symlink in %s).", action, service, src_path);
> +        }
> +    }
> +
> +    free(filepattern);
> +
> +    return r;
> +}
> +
> +// Print to stdout wether <service> is enabled or disabled on boot
> +// Prints <service> as Not available when initscript is not found
> +// in either enabled_path or disabled_path.
> +void print_boot_status(char *service) {
> +    char *filepattern = malloc((3 + strlen(service) + 1) * sizeof(char));
> +    sprintf(filepattern, "S??%s", service);
> +    char *enabled = find_file_in_dir(enabled_path, filepattern);
> +    char *disabled = find_file_in_dir(disabled_path, filepattern);
> +
> +    if (enabled != NULL)
> +        fprintf(stdout, "%s is enabled on boot.\n", service);
> +    else if (disabled != NULL)
> +        fprintf(stdout, "%s is disabled on boot.\n", service);
> +    else
> +        fprintf(stdout, "%s is not available for boot. (Service has no valid symlink in either %s or %s).\n", service, enabled_path, disabled_path);
> +
> +    free(filepattern);
> +}
> +
>   int main(int argc, char *argv[]) {
> -	char command[BUFFER_SIZE];
> -
> -	if (!(initsetuid()))
> -		exit(1);
> -
> -	if (argc < 3) {
> -		fprintf(stderr, "\nMissing arguments.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -		exit(1);
> -	}
> -
> -	const char* name = argv[1];
> -
> -	if (strlen(name) > 32) {
> -	    fprintf(stderr, "\nString to large.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -	    exit(1);
> -	}
> -
> -	// Check if the input argument is valid
> -	if (!is_valid_argument_alnum(name)) {
> -		fprintf(stderr, "Invalid add-on name: %s\n", name);
> -		exit(2);
> -	}
> -
> -	sprintf(command, "/opt/pakfire/db/installed/meta-%s", name);
> -	FILE *fp = fopen(command,"r");
> -	if ( fp ) {
> -	    fclose(fp);
> -	} else {
> -	    fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl addon (start|stop|restart|reload|status|enable|disable)\n\n", name);
> -	    exit(1);
> -	}
> -
> -	if (strcmp(argv[2], "start") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s start", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "stop") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s stop", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "restart") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s restart", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "reload") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s reload", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "status") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s status", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "enable") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "disable") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "mkdir -p /etc/rc.d/rc3.d/off");
> -		safe_system(command);
> -		snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
> -		safe_system(command);
> -	} else {
> -		fprintf(stderr, "\nBad argument given.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -		exit(1);
> -	}
> -
> -	return 0;
> +    char **services = NULL;
> +    char **services_ptr = NULL;
> +    int servicescnt = 0;
> +    char *addon = argv[1];
> +    char *action = argv[2];
> +    char *service_filter = NULL;
> +    int actioned = 0;
> +    int r = 0;
> +
> +    if (!(initsetuid()))
> +        exit(1);
> +
> +    if (argc < 3) {
> +        fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage);
> +        exit(1);
> +    }
> +
> +    if (argc == 4)
> +        service_filter = argv[3];
> +
> +    if (strlen(addon) > 32) {
> +        fprintf(stderr, "\nString to large.\n\n%s\n\n", usage);
> +        exit(1);
> +    }
> +
> +    // Check if the input argument is valid
> +    if (!is_valid_argument_alnum(addon)) {
> +        fprintf(stderr, "Invalid add-on name: %s.\n", addon);
> +        exit(2);
> +    }
> +
> +    // Get initscript name(s) from addon metadata
> +    services_ptr = get_addon_services(addon, &servicescnt);
> +    services = services_ptr;
> +    if (services == NULL || *services == 0) {
> +        if (strcmp(errormsg, "") != 0)
> +            fprintf(stderr, "\n%s\n\n", errormsg);
> +        else
> +            fprintf(stderr, "\nAddon '%s' has no services.\n\n", addon);
> +        exit(1);
> +    }
> +
> +    if (strcmp(action, "start") == 0 ||
> +        strcmp(action, "stop") == 0 ||
> +        strcmp(action, "restart") == 0 ||
> +        strcmp(action, "reload") == 0 ||
> +        strcmp(action, "status") == 0) {
> +
> +        while (*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) ||
> +                 service_filter == NULL) {
> +                if (initscript_action(*services, action) != 0)
> +                    r = 1;
> +                ++actioned;
> +            }
> +            ++services;
> +        }
> +
> +    } else if (strcmp(action, "enable") == 0 ||
> +               strcmp(action, "disable") == 0) {
> +
> +        while (*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) ||
> +                 service_filter == NULL) {
> +                if (toggle_service(*services, action) == 0) {
> +                    fprintf(stdout, "%sd service %s\n", action, *services);
> +                }
> +                else {
> +                    r = 1;
> +                    fprintf(stderr, "\n%s\n\n", errormsg);
> +                }
> +
> +                ++actioned;
> +            }
> +            ++services;
> +        }
> +
> +    } else if (strcmp(action, "boot-status") == 0) {
> +        while(*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) ||
> +                 service_filter == NULL) {
> +                print_boot_status(*services);
> +                ++actioned;
> +            }
> +            ++services;
> +        }
> +
> +    } else if (strcmp(action, "list-services") == 0) {
> +        fprintf(stdout, "\nServices for addon %s:\n", addon);
> +        while (*services != 0) {
> +            fprintf(stdout, "  %s\n", *services);
> +            ++actioned;
> +            ++services;
> +        }
> +        fprintf(stdout, "\n");
> +
> +    } else {
> +        fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage);
> +        r = 1;
> +    }
> +
> +    if (r == 0 && service_filter != NULL && actioned == 0) {
> +        fprintf(stderr, "\nNo service %s found for addon %s. Use 'list-services' to get a list of available services\n\n%s\n\n", service_filter, addon, usage);
> +        r = 1;
> +    }
> +
> +    // Cleanup
> +    for(int i = 0; i < servicescnt; i++)
> +        free(services_ptr[i]);
> +    free(services_ptr);
> +
> +    return r;
>   }
  

Hello Robin,

This is a little bit harder to review...

> On 3 Oct 2022, at 16:27, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> * Addonctrl will now check in addon metadata for the exact initscript
>  names (Services). If more than one initscript is defined for an addon,
>  the requested action will be performed on all listed initscripts.
> * Added posibility to perform action on a specific initscript of an
>  addon instead of on all initscripts of the addon.

Is this actually used anywhere? I cannot come up with an example where this would be relevant.

> * New action 'list-services' to display a list of services related to
>  an addon.

Agreed.

> * New action 'boot-status' to display wether service(s) are enabled
>  to start on boot or not.

Agreed.

> * More error checking and cleaner error reporting to user

I like this :)

> * General cleanup and code restructuring to avoid code duplication
> * Updated and made usage instructions more verbose.
> 
> Fixes: Bug#12935
> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> ---
> src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++------
> 1 file changed, 323 insertions(+), 61 deletions(-)
> 
> diff --git a/src/misc-progs/addonctrl.c b/src/misc-progs/addonctrl.c
> index 14b4b1325..277bd1809 100644
> --- a/src/misc-progs/addonctrl.c
> +++ b/src/misc-progs/addonctrl.c
> @@ -10,71 +10,333 @@
> #include <string.h>
> #include <unistd.h>
> #include <sys/types.h>
> +#include <sys/stat.h>
> #include <fcntl.h>
> +#include <dirent.h>
> +#include <errno.h>
> #include "setuid.h"
> 
> #define BUFFER_SIZE 1024
> 
> +const char *enabled_path = "/etc/rc.d/rc3.d";
> +const char *disabled_path = "/etc/rc.d/rc3.d/off";
> +
> +char errormsg[BUFFER_SIZE] = "";
> +char *usage = 
> +    "Usage\n"
> +    "  addonctrl <addon> (start|stop|restart|reload|enable|disable|status|boot-status|list-services) [<service>]\n"
> +    "\n"
> +    "Options:\n"
> +    "  <addon>\t\tName of the addon to control\n"
> +    "  <service>\t\tSpecific service of the addon to control (optional)\n"
> +    "  \t\t\tBy default the requested action is performed on all related services. See also 'list-services'.\n"
> +    "  start\t\t\tStart service(s) of the addon\n"
> +    "  stop\t\t\tStop service(s) of the addon\n"
> +    "  restart\t\tRestart service(s) of the addon\n"
> +    "  enable\t\tEnable service(s) of the addon to start at boot\n"
> +    "  disable\t\tDisable service(s) of the addon to start at boot\n"
> +    "  status\t\tDisplay current state of addon service(s)\n"
> +    "  boot-status\t\tDisplay wether service(s) is enabled on boot or not\n"
> +    "  list-services\t\tDisplay a list of services related to the addon";

This string could be const, but I am sure the compiler would assume that anyways.

> +
> +// Check if <text> matches <pattern>. Wildcard '?' matches any single character.
> +// Returns 1 when found, 0 when not found
> +int match(const char *pattern, const char *text) {
> +    if (pattern[0] == '\0' && *text == '\0')
> +        return 1;

Do you not want to check whether you have reached the end of either string?

I would have written it as:

  if (!*pattern || !*text)
    return 1;


> +    if (*text != '\0' && (pattern[0]=='?' || pattern[0]==*text))
> +        return match(pattern+1, text+1);
> +    return 0;

I have nothing against a recursive implementation, but potentially, a for loop would have been easier to read:

for (unsigned int i = 0; i < strlen(text); i++) {
	// ? matches anything
	if (pattern[i] == '?')
		continue;

	// Check for mismatch
	if (pattern[i] != text[i])
		return 1;
}

return 0;

This example does not include checking whether pattern is at least as long as text.

Since this function is only being used here, you could have declared it as static. The compiler might do this for you.

> +}
> +
> +// Find a file <path> using <filepattern> allowing the '?' wildcard.
> +// Returns the found filename or NULL if not found
> +char* find_file_in_dir(const char *path, const char *filepattern) 
> +{
> +    static struct dirent *entry;
> +    DIR *dp;
> +    int found = 0;
> +
> +    if ((dp = opendir(path)) != NULL) {
> +        while(found == 0 && (entry = readdir(dp)) != NULL)
> +            found = match(filepattern, entry->d_name);
> +

Now, reading through this, I understand what the match function is used for :)

There is already a function for this:

  https://man7.org/linux/man-pages/man3/fnmatch.3.html

I think it works exactly the way you want this.

> +        closedir(dp);
> +    }
> +
> +    if (! found) {
> +        return NULL;
> +    }
> +
> +    return entry->d_name;

I am not sure where you can rely on closedir() not deallocating the entry.

It would be best to change found to “char *” and call strdup(entry->d_name) after you found a match.

You can then simply return found at the end which should have been initialised to NULL.

> +}
> +
> +// Reads Services metadata for <addon>.
> +// Returns pointer to array of strings containing the services for <addon> 
> +// and sets <servicescnt> to the number of found services
> +char **get_addon_services(const char *addon, int *servicescnt) {
> +    const char *metafile_prefix = "/opt/pakfire/db/installed/meta-";
> +    const char *metadata_key = "Services";
> +    const char *keyvalue_delim = ":";
> +    const char *service_delim = " ";
> +    char *token;
> +    char **services = NULL;
> +    char *service;
> +    char line[BUFFER_SIZE];
> +    int i = 0;

Since you are passing “addon” to strlen(), I wouldn’t mind checking that it isn’t NULL, like so:

  if (!addon) {
    errno = EINVAL;
    return NULL;
  }

> +    char *metafile = malloc((strlen(metafile_prefix) + strlen(addon) + 1) * sizeof(char));

Why do you multiply this by the size of char? malloc() already takes bytes.

You should also check whether you could successfully allocate the memory and if not, return NULL.

> + 
> +    sprintf(metafile, "%s%s", metafile_prefix, addon);

Alternatively, you can use asprintf() which will automatically allocate a buffer of the correct size.

> +    FILE *fp = fopen(metafile,"r");
> +    if ( fp ) {
> +        // Get initscript(s) for addon from meta-file
> +        while (!feof(fp) && services == NULL) {
> +            if (fgets(line, BUFFER_SIZE - 1, fp) != NULL) {
> +                // Strip newline
> +                char *newline = strchr(line, '\n');
> +                if (newline) *newline = 0;

There is a getline() function which would combine this whole block starting from fgets():

  https://man7.org/linux/man-pages/man3/getline.3.html

getline() will also automatically allocate a buffer of the correct size for each line.

> +
> +                // Parse key/value and look for required key.
> +                token = strtok(line, keyvalue_delim);
> +                if (token != NULL && strcmp(token, metadata_key) == 0) {
> +                    token = strtok(NULL, keyvalue_delim);
> +                    if (token != NULL) {
> +                        services = malloc((strlen(token) + 1) * sizeof (char *));

Same as above. The multiplication is not required I believe.

> +                        // Put each service in services array
> +                        service = strtok(token, service_delim);
> +                        while (service != NULL) {
> +                            services[i] = malloc((strlen(service) + 1) * sizeof (char));
> +                            strcpy(services[i++], service);

Those two lines could just be replaced by using strdup().

> +
> +                            service = strtok(NULL, service_delim);
> +                        }

I am not sure how you are allocating services. It is kind of like a worst-case scenario, that you are doing here. I can live with this, because this is a very short-lived program and wasting a couple of extra bytes of memory is not going to be a disaster.

But you could also use reallocarray() in the inner while loop and only allocate as much memory as you actually need:

  https://man.archlinux.org/man/reallocarray.3bsd.en

> +                    }
> +                }
> +            } else {
> +                snprintf(errormsg, BUFFER_SIZE - 1, "Could not read '%s' metadata for addon '%s'.", metadata_key, addon);
> +            }
> +        }
> +        fclose(fp);
> +    }  else {
> +        snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not found.\n\n%s", addon, usage);
> +    }
> +
> +    free (metafile);
> +    *servicescnt = i;
> +    return services;
> +}
> +
> +// Calls initscript <service> with parameter <action>
> +int initscript_action(const char *service, const char *action) {
> +    const char *initd_path = "/etc/rc.d/init.d";
> +    char *command = malloc((strlen(initd_path) + 1 + strlen(service) + 1) * sizeof(char));
> +    int r = 0;

This function is opening us up for injecting any command from the package metadata and run it as root.

If “service” for example is “samba; reboot”, then the initscript that you would want to launch is actually launched first and then the following reboot command. This cannot stay that way.

You will need to use run() from setuid.c which is not launching a shell and is therefore making this safer.

run() will take the initscript as first argument and any other parameters as an array. So it could look like this:

int initscript_action(const char *service, const char *action) {
	char buffer[PATH_MAX];
	int r;

	r = snprintf(buffer, sizeof(buffer), "/etc/rc.d/init.d/%s", service);
	if (r < 0)
		return r;

	const char* argv[] = {
		action,
		NULL
	};

	return run(buffer, argv);
}

I know that you are checking inputs further down below, but I do not want to take the risk that something (even after some more changes to this file) is slipping through making us vulnerable that severely.

> +
> +    sprintf(command, "%s/%s %s", initd_path, service, action);
> +    r = safe_system(command);
> +    free(command);
> +
> +    return r; 
> +}
> +
> +// Move an initscript with filepattern from <src_path> to <dest_path>
> +// Returns:
> +//   -1: Error during move. Details in errno (returned by C rename function)
> +//   0: Success
> +//   1: file was not moved, but is already in <dest_path>
> +//   2: file does not exist in either in <src_path> or <dest_path>
> +int move_initscript_by_pattern(const char *src_path, const char *dest_path, const char *filepattern) {
> +    char *src, *dest;
> +    int r = 1;
> +    char *filename = find_file_in_dir(src_path, filepattern);
> +
> +    if ( filename != NULL ) {
> +        src = malloc((strlen(src_path) + 1 + strlen(filename) + 1) * sizeof(char));
> +        dest = malloc((strlen(dest_path) + 1 + strlen(filename) + 1) * sizeof(char));
> +        sprintf(src, "%s/%s", src_path, filename);
> +        sprintf(dest, "%s/%s", dest_path, filename);

See my example above how to allocate a string (or actually not do that at all by using a stack variable).

If allocation fails for some reason, your code will try to write to a NULL pointer and later free a NULL pointer.

> +
> +        r = rename(src, dest);
> +
> +        free(src);
> +        free(dest);
> +    } else {
> +        filename = find_file_in_dir(dest_path, filepattern);
> +        if (filename == NULL)
> +            r = 2;
> +    }
> +
> +    return r;
> +}
> +
> +// Enable/Disable addon service(s) by moving initscript symlink from/to disabled_path
> +int toggle_service(const char *service, const char *action) {
> +    const char *src_path, *dest_path;
> +    char *filepattern = malloc((3 + strlen(service) + 1) * sizeof(char));
> +    int r = 0;
> +    
> +    sprintf(filepattern, "S??%s", service);

See above.

> +
> +    if (strcmp(action, "enable") == 0) {
> +        src_path = disabled_path;
> +        dest_path = enabled_path;
> +    } else {
> +        src_path = enabled_path;
> +        dest_path = disabled_path;
> +    }
> +
> +    // Ensure disabled_path exists
> +    if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + S_IROTH + S_IXOTH) == -1 && errno != EEXIST) {
> +        r = 1;
> +        snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s. (Error: %d)", disabled_path, errno);
> +    } else {
> +        r = move_initscript_by_pattern(src_path, dest_path, filepattern);
> +        if (r == -1 ) {
> +            r = 1;
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. (Error: %d)", action, service, errno);
> +        } else if (r == 1) {
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is already %sd. Skipping...", service, action);
> +        } else if (r == 2) {
> +            snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s service %s. (Service has no valid symlink in %s).", action, service, src_path);
> +        }
> +    }
> +
> +    free(filepattern);
> +    
> +    return r;
> +}
> +
> +// Print to stdout wether <service> is enabled or disabled on boot
> +// Prints <service> as Not available when initscript is not found
> +// in either enabled_path or disabled_path.
> +void print_boot_status(char *service) {
> +    char *filepattern = malloc((3 + strlen(service) + 1) * sizeof(char));
> +    sprintf(filepattern, "S??%s", service);

See above.

> +    char *enabled = find_file_in_dir(enabled_path, filepattern);
> +    char *disabled = find_file_in_dir(disabled_path, filepattern);
> +
> +    if (enabled != NULL) 
> +        fprintf(stdout, "%s is enabled on boot.\n", service);
> +    else if (disabled != NULL) 
> +        fprintf(stdout, "%s is disabled on boot.\n", service);
> +    else
> +        fprintf(stdout, "%s is not available for boot. (Service has no valid symlink in either %s or %s).\n", service, enabled_path, disabled_path);
> +
> +    free(filepattern);
> +}
> +
> int main(int argc, char *argv[]) {
> -	char command[BUFFER_SIZE];
> -
> -	if (!(initsetuid()))
> -		exit(1);
> -
> -	if (argc < 3) {
> -		fprintf(stderr, "\nMissing arguments.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -		exit(1);
> -	}
> -
> -	const char* name = argv[1];
> -
> -	if (strlen(name) > 32) {
> -	    fprintf(stderr, "\nString to large.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -	    exit(1);
> -	}
> -
> -	// Check if the input argument is valid
> -	if (!is_valid_argument_alnum(name)) {
> -		fprintf(stderr, "Invalid add-on name: %s\n", name);
> -		exit(2);
> -	}
> -
> -	sprintf(command, "/opt/pakfire/db/installed/meta-%s", name);
> -	FILE *fp = fopen(command,"r");
> -	if ( fp ) {
> -	    fclose(fp);
> -	} else {
> -	    fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl addon (start|stop|restart|reload|status|enable|disable)\n\n", name);
> -	    exit(1);
> -	}
> -
> -	if (strcmp(argv[2], "start") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s start", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "stop") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s stop", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "restart") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s restart", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "reload") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s reload", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "status") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s status", name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "enable") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
> -		safe_system(command);
> -	} else if (strcmp(argv[2], "disable") == 0) {
> -		snprintf(command, BUFFER_SIZE - 1, "mkdir -p /etc/rc.d/rc3.d/off");
> -		safe_system(command);
> -		snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
> -		safe_system(command);
> -	} else {
> -		fprintf(stderr, "\nBad argument given.\n\naddonctrl addon (start|stop|restart|reload|enable|disable)\n\n");
> -		exit(1);
> -	}
> -
> -	return 0;
> +    char **services = NULL;
> +    char **services_ptr = NULL;
> +    int servicescnt = 0;
> +    char *addon = argv[1];
> +    char *action = argv[2];
> +    char *service_filter = NULL;
> +    int actioned = 0;
> +    int r = 0;
> +
> +    if (!(initsetuid()))
> +        exit(1);
> +
> +    if (argc < 3) {
> +        fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage);
> +        exit(1);
> +    }
> +
> +    if (argc == 4) 
> +        service_filter = argv[3];
> +
> +    if (strlen(addon) > 32) {
> +        fprintf(stderr, "\nString to large.\n\n%s\n\n", usage);
> +        exit(1);
> +    }

How is 32 chosen?

> +
> +    // Check if the input argument is valid
> +    if (!is_valid_argument_alnum(addon)) {
> +        fprintf(stderr, "Invalid add-on name: %s.\n", addon);
> +        exit(2);
> +    }
> +
> +    // Get initscript name(s) from addon metadata
> +    services_ptr = get_addon_services(addon, &servicescnt);
> +    services = services_ptr;
> +    if (services == NULL || *services == 0) {
> +        if (strcmp(errormsg, "") != 0)
> +            fprintf(stderr, "\n%s\n\n", errormsg);
> +        else
> +            fprintf(stderr, "\nAddon '%s' has no services.\n\n", addon);
> +        exit(1);
> +    }
> +
> +    if (strcmp(action, "start") == 0 ||
> +        strcmp(action, "stop") == 0 ||
> +        strcmp(action, "restart") == 0 ||
> +        strcmp(action, "reload") == 0 ||
> +        strcmp(action, "status") == 0) {
> +
> +        while (*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) || 
> +                 service_filter == NULL) {
> +                if (initscript_action(*services, action) != 0) 
> +                    r = 1;
> +                ++actioned;

Wouldn’t it be a better ideal to pass the service_filter argument to the action function and only filter once in that function?

Otherwise you are copying the same code over and over again. Or have a helper function that will be called from initscript_action, toggle_service and so on?

> +            }
> +            ++services;
> +        }
> +
> +    } else if (strcmp(action, "enable") == 0 ||
> +               strcmp(action, "disable") == 0) {
> +
> +        while (*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) || 
> +                 service_filter == NULL) {
> +                if (toggle_service(*services, action) == 0) {
> +                    fprintf(stdout, "%sd service %s\n", action, *services);
> +                }
> +                else {
> +                    r = 1;
> +                    fprintf(stderr, "\n%s\n\n", errormsg);
> +                }
> +                
> +                ++actioned;
> +            }
> +            ++services;
> +        }
> +
> +    } else if (strcmp(action, "boot-status") == 0) {
> +        while(*services != 0) {
> +            if ((service_filter != NULL && strcmp(service_filter, *services) == 0) || 
> +                 service_filter == NULL) {
> +                print_boot_status(*services);
> +                ++actioned;
> +            }
> +            ++services;
> +        }
> +    
> +    } else if (strcmp(action, "list-services") == 0) {
> +        fprintf(stdout, "\nServices for addon %s:\n", addon);
> +        while (*services != 0) {
> +            fprintf(stdout, "  %s\n", *services);
> +            ++actioned;
> +            ++services;
> +        }
> +        fprintf(stdout, "\n");
> +
> +    } else {
> +        fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage);
> +        r = 1;
> +    }
> +
> +    if (r == 0 && service_filter != NULL && actioned == 0) {
> +        fprintf(stderr, "\nNo service %s found for addon %s. Use 'list-services' to get a list of available services\n\n%s\n\n", service_filter, addon, usage);
> +        r = 1;
> +    }
> +
> +    // Cleanup
> +    for(int i = 0; i < servicescnt; i++) 
> +        free(services_ptr[i]);
> +    free(services_ptr);
> +
> +    return r;
> }
> -- 
> 2.37.3

So all in all, this is the way to go. Since we have all this metadata now, we should use it, and this is a very good place to start.

However, there are some problems in this implementation and since this code is gaining root privileges, we need to make sure that it is 100% safe. Safe against any garbage inputs that might have ended up here, safe against some making some easy mistakes when the script is being changed again next time. So basically, do our best here. Sorry to be so strict, but the setuid binaries are a massive pain point in the current web UI and therefore we now have the great pleasure to fix old design issues.

-Michael

> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  

Hi Michael

Michael Tremer schreef op di 04-10-2022 om 11:28 [+0100]:
> Hello Robin,
> 
> This is a little bit harder to review...
I assumed so, as it is practically a full rewrite :-).
for many of your comments, I will have to investigate and educate
myself a bit more. I'll mark them when with 'will check' for now :-)

> 
> > On 3 Oct 2022, at 16:27, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > * Addonctrl will now check in addon metadata for the exact
> > initscript
> >  names (Services). If more than one initscript is defined for an
> > addon,
> >  the requested action will be performed on all listed initscripts.
> > * Added posibility to perform action on a specific initscript of an
> >  addon instead of on all initscripts of the addon.
> 
> Is this actually used anywhere? I cannot come up with an example
> where this would be relevant.
Yes, this is the only way services.cgi would be able to
start/stop/enable/disable single services instead of all services of an
addon.
As discussed with Adolf in the bug report, we wanted to give users fine
control of which services to stop/start/enable/disable on service-level
and not on addon-level. 
When, for example a users wants a single service of a multi-service
addon to be disabled, I think he should be able to. Also with addon
libvirt which has 2 services libvirtd and virtlogd, if one of those 2
hangs or starts acting weirdly a user should be able to restart that
specific service.
(this, for example, also opens up the possibility of adding a user
optional suspend/resume guests initscript to that addon in the future)

> 
> > * New action 'list-services' to display a list of services related
> > to
> >  an addon.
> 
> Agreed.
> 
> > * New action 'boot-status' to display wether service(s) are enabled
> >  to start on boot or not.
> 
> Agreed.
> 
> > * More error checking and cleaner error reporting to user
> 
> I like this :)
> 
> > * General cleanup and code restructuring to avoid code duplication
> > * Updated and made usage instructions more verbose.
> > 
> > Fixes: Bug#12935
> > Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> > ---
> > src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++---
> > ---
> > 1 file changed, 323 insertions(+), 61 deletions(-)
> > 
> > diff --git a/src/misc-progs/addonctrl.c b/src/misc-
> > progs/addonctrl.c
> > index 14b4b1325..277bd1809 100644
> > --- a/src/misc-progs/addonctrl.c
> > +++ b/src/misc-progs/addonctrl.c
> > @@ -10,71 +10,333 @@
> > #include <string.h>
> > #include <unistd.h>
> > #include <sys/types.h>
> > +#include <sys/stat.h>
> > #include <fcntl.h>
> > +#include <dirent.h>
> > +#include <errno.h>
> > #include "setuid.h"
> > 
> > #define BUFFER_SIZE 1024
> > 
> > +const char *enabled_path = "/etc/rc.d/rc3.d";
> > +const char *disabled_path = "/etc/rc.d/rc3.d/off";
> > +
> > +char errormsg[BUFFER_SIZE] = "";
> > +char *usage = 
> > +    "Usage\n"
> > +    "  addonctrl <addon>
> > (start|stop|restart|reload|enable|disable|status|boot-status|list-
> > services) [<service>]\n"
> > +    "\n"
> > +    "Options:\n"
> > +    "  <addon>\t\tName of the addon to control\n"
> > +    "  <service>\t\tSpecific service of the addon to control
> > (optional)\n"
> > +    "  \t\t\tBy default the requested action is performed on all
> > related services. See also 'list-services'.\n"
> > +    "  start\t\t\tStart service(s) of the addon\n"
> > +    "  stop\t\t\tStop service(s) of the addon\n"
> > +    "  restart\t\tRestart service(s) of the addon\n"
> > +    "  enable\t\tEnable service(s) of the addon to start at
> > boot\n"
> > +    "  disable\t\tDisable service(s) of the addon to start at
> > boot\n"
> > +    "  status\t\tDisplay current state of addon service(s)\n"
> > +    "  boot-status\t\tDisplay wether service(s) is enabled on boot
> > or not\n"
> > +    "  list-services\t\tDisplay a list of services related to the
> > addon";
> 
> This string could be const, but I am sure the compiler would assume
> that anyways.

Probably, but I'll make it const manually.

> 
> > +
> > +// Check if <text> matches <pattern>. Wildcard '?' matches any
> > single character.
> > +// Returns 1 when found, 0 when not found
> > +int match(const char *pattern, const char *text) {
> > +    if (pattern[0] == '\0' && *text == '\0')
> > +        return 1;
> 
> Do you not want to check whether you have reached the end of either
> string?
> 
> I would have written it as:
> 
>   if (!*pattern || !*text)
>     return 1;
> 
both have to be at their end at the same time, otherwise it should not
be a match. If pattern is at the end but text is not, it would also
match filenames that have additional characters after the matched part.
So in the hypothetical case that there were 2 services with a mutual
beginning of the filename: for example 'zabbix' and 'zabbix_agentd'.
Matching for 'zabbix' would also match 'zabbix_agentd'.
Not that there currently are such cases in all possible initscripts, I
think. But a user could have added a custom initscript witch such name.

> 
> > +    if (*text != '\0' && (pattern[0]=='?' || pattern[0]==*text))
> > +        return match(pattern+1, text+1);
> > +    return 0;
> 
> I have nothing against a recursive implementation, but potentially, a
> for loop would have been easier to read:
> 
> for (unsigned int i = 0; i < strlen(text); i++) {
>         // ? matches anything
>         if (pattern[i] == '?')
>                 continue;
> 
>         // Check for mismatch
>         if (pattern[i] != text[i])
>                 return 1;
> }
> 
> return 0;
> 
> This example does not include checking whether pattern is at least as
> long as text.
> 
> Since this function is only being used here, you could have declared
> it as static. The compiler might do this for you.
> 
> > +}
> > +
> > +// Find a file <path> using <filepattern> allowing the '?'
> > wildcard.
> > +// Returns the found filename or NULL if not found
> > +char* find_file_in_dir(const char *path, const char *filepattern) 
> > +{
> > +    static struct dirent *entry;
> > +    DIR *dp;
> > +    int found = 0;
> > +
> > +    if ((dp = opendir(path)) != NULL) {
> > +        while(found == 0 && (entry = readdir(dp)) != NULL)
> > +            found = match(filepattern, entry->d_name);
> > +
> 
> Now, reading through this, I understand what the match function is
> used for :)
> 
> There is already a function for this:
> 
>   https://man7.org/linux/man-pages/man3/fnmatch.3.html
> 
> I think it works exactly the way you want this.

I was not aware of that function. As pointed out in the past and in the
bug report; other than when I patched getipstat, my C coding skills
have not been used for almost 25 year :-). I have been searching the
web for a function like this but didn't find fnmach. I will try using
that instead.

> 
> > +        closedir(dp);
> > +    }
> > +
> > +    if (! found) {
> > +        return NULL;
> > +    }
> > +
> > +    return entry->d_name;
> 
> I am not sure where you can rely on closedir() not deallocating the
> entry.
> 
> It would be best to change found to “char *” and call strdup(entry-
> >d_name) after you found a match.
> 
> You can then simply return found at the end which should have been
> initialised to NULL.

Agreed

> 
> > +}
> > +
> > +// Reads Services metadata for <addon>.
> > +// Returns pointer to array of strings containing the services for
> > <addon> 
> > +// and sets <servicescnt> to the number of found services
> > +char **get_addon_services(const char *addon, int *servicescnt) {
> > +    const char *metafile_prefix = "/opt/pakfire/db/installed/meta-
> > ";
> > +    const char *metadata_key = "Services";
> > +    const char *keyvalue_delim = ":";
> > +    const char *service_delim = " ";
> > +    char *token;
> > +    char **services = NULL;
> > +    char *service;
> > +    char line[BUFFER_SIZE];
> > +    int i = 0;
> 
> Since you are passing “addon” to strlen(), I wouldn’t mind checking
> that it isn’t NULL, like so:
> 
>   if (!addon) {
>     errno = EINVAL;
>     return NULL;
>   }
> 
will do

> > +    char *metafile = malloc((strlen(metafile_prefix) +
> > strlen(addon) + 1) * sizeof(char));
> 
> Why do you multiply this by the size of char? malloc() already takes
> bytes.

True, but I read a recommendation somewhere to explicitly do this for
clarity. 
Also I've read that char could be larger when UTF-8 is in play
depending on the implementation of C used. Which is probably not the
case here.

> 
> You should also check whether you could successfully allocate the
> memory and if not, return NULL.

agreed, better safe than sorry

> 
> > + 
> > +    sprintf(metafile, "%s%s", metafile_prefix, addon);
> 
> Alternatively, you can use asprintf() which will automatically
> allocate a buffer of the correct size.

Will check 

> 
> > +    FILE *fp = fopen(metafile,"r");
> > +    if ( fp ) {
> > +        // Get initscript(s) for addon from meta-file
> > +        while (!feof(fp) && services == NULL) {
> > +            if (fgets(line, BUFFER_SIZE - 1, fp) != NULL) {
> > +                // Strip newline
> > +                char *newline = strchr(line, '\n');
> > +                if (newline) *newline = 0;
> 
> There is a getline() function which would combine this whole block
> starting from fgets():
> 
>   https://man7.org/linux/man-pages/man3/getline.3.html
> 
> getline() will also automatically allocate a buffer of the correct
> size for each line.

will check

> 
> > +
> > +                // Parse key/value and look for required key.
> > +                token = strtok(line, keyvalue_delim);
> > +                if (token != NULL && strcmp(token, metadata_key)
> > == 0) {
> > +                    token = strtok(NULL, keyvalue_delim);
> > +                    if (token != NULL) {
> > +                        services = malloc((strlen(token) + 1) *
> > sizeof (char *));
> 
> Same as above. The multiplication is not required I believe.
> 
> > +                        // Put each service in services array
> > +                        service = strtok(token, service_delim);
> > +                        while (service != NULL) {
> > +                            services[i] = malloc((strlen(service)
> > + 1) * sizeof (char));
> > +                            strcpy(services[i++], service);
> 
> Those two lines could just be replaced by using strdup().

Will check

> 
> > +
> > +                            service = strtok(NULL, service_delim);
> > +                        }
> 
> I am not sure how you are allocating services. It is kind of like a
> worst-case scenario, that you are doing here. I can live with this,
> because this is a very short-lived program and wasting a couple of
> extra bytes of memory is not going to be a disaster.
> 
> But you could also use reallocarray() in the inner while loop and
> only allocate as much memory as you actually need:
> 
>   https://man.archlinux.org/man/reallocarray.3bsd.en

I have poured quite a bit of sweat trying to implement this :-). I will
check reallocarray. I'm all for cleaner/better code.

> 
> > +                    }
> > +                }
> > +            } else {
> > +                snprintf(errormsg, BUFFER_SIZE - 1, "Could not
> > read '%s' metadata for addon '%s'.", metadata_key, addon);
> > +            }
> > +        }
> > +        fclose(fp);
> > +    }  else {
> > +        snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not
> > found.\n\n%s", addon, usage);
> > +    }
> > +
> > +    free (metafile);
> > +    *servicescnt = i;
> > +    return services;
> > +}
> > +
> > +// Calls initscript <service> with parameter <action>
> > +int initscript_action(const char *service, const char *action) {
> > +    const char *initd_path = "/etc/rc.d/init.d";
> > +    char *command = malloc((strlen(initd_path) + 1 +
> > strlen(service) + 1) * sizeof(char));
> > +    int r = 0;
> 
> This function is opening us up for injecting any command from the
> package metadata and run it as root.
> 
> If “service” for example is “samba; reboot”, then the initscript that
> you would want to launch is actually launched first and then the
> following reboot command. This cannot stay that way.
> 
> You will need to use run() from setuid.c which is not launching a
> shell and is therefore making this safer.
> 
> run() will take the initscript as first argument and any other
> parameters as an array. So it could look like this:
> 
> int initscript_action(const char *service, const char *action) {
>         char buffer[PATH_MAX];
>         int r;
> 
>         r = snprintf(buffer, sizeof(buffer), "/etc/rc.d/init.d/%s",
> service);
>         if (r < 0)
>                 return r;
> 
>         const char* argv[] = {
>                 action,
>                 NULL
>         };
> 
>         return run(buffer, argv);
> }
> 
> I know that you are checking inputs further down below, but I do not
> want to take the risk that something (even after some more changes to
> this file) is slipping through making us vulnerable that severely.

I more or less recycled the method used in the original code which also
first created a command-string including executable and parameters and
passing it to safe_system. Also not knowing about the run function.
But run sounds indeed better and safer, so I'll go with that indeed.

> 
> > +
> > +    sprintf(command, "%s/%s %s", initd_path, service, action);
> > +    r = safe_system(command);
> > +    free(command);
> > +
> > +    return r; 
> > +}
> > +
> > +// Move an initscript with filepattern from <src_path> to
> > <dest_path>
> > +// Returns:
> > +//   -1: Error during move. Details in errno (returned by C rename
> > function)
> > +//   0: Success
> > +//   1: file was not moved, but is already in <dest_path>
> > +//   2: file does not exist in either in <src_path> or <dest_path>
> > +int move_initscript_by_pattern(const char *src_path, const char
> > *dest_path, const char *filepattern) {
> > +    char *src, *dest;
> > +    int r = 1;
> > +    char *filename = find_file_in_dir(src_path, filepattern);
> > +
> > +    if ( filename != NULL ) {
> > +        src = malloc((strlen(src_path) + 1 + strlen(filename) + 1)
> > * sizeof(char));
> > +        dest = malloc((strlen(dest_path) + 1 + strlen(filename) +
> > 1) * sizeof(char));
> > +        sprintf(src, "%s/%s", src_path, filename);
> > +        sprintf(dest, "%s/%s", dest_path, filename);
> 
> See my example above how to allocate a string (or actually not do
> that at all by using a stack variable).
> 

will check

> If allocation fails for some reason, your code will try to write to a
> NULL pointer and later free a NULL pointer.
> 
> > +
> > +        r = rename(src, dest);
> > +
> > +        free(src);
> > +        free(dest);
> > +    } else {
> > +        filename = find_file_in_dir(dest_path, filepattern);
> > +        if (filename == NULL)
> > +            r = 2;
> > +    }
> > +
> > +    return r;
> > +}
> > +
> > +// Enable/Disable addon service(s) by moving initscript symlink
> > from/to disabled_path
> > +int toggle_service(const char *service, const char *action) {
> > +    const char *src_path, *dest_path;
> > +    char *filepattern = malloc((3 + strlen(service) + 1) *
> > sizeof(char));
> > +    int r = 0;
> > +    
> > +    sprintf(filepattern, "S??%s", service);
> 
> See above.
> 
> > +
> > +    if (strcmp(action, "enable") == 0) {
> > +        src_path = disabled_path;
> > +        dest_path = enabled_path;
> > +    } else {
> > +        src_path = enabled_path;
> > +        dest_path = disabled_path;
> > +    }
> > +
> > +    // Ensure disabled_path exists
> > +    if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + S_IROTH
> > + S_IXOTH) == -1 && errno != EEXIST) {
> > +        r = 1;
> > +        snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s.
> > (Error: %d)", disabled_path, errno);
> > +    } else {
> > +        r = move_initscript_by_pattern(src_path, dest_path,
> > filepattern);
> > +        if (r == -1 ) {
> > +            r = 1;
> > +            snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s.
> > (Error: %d)", action, service, errno);
> > +        } else if (r == 1) {
> > +            snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is
> > already %sd. Skipping...", service, action);
> > +        } else if (r == 2) {
> > +            snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s
> > service %s. (Service has no valid symlink in %s).", action,
> > service, src_path);
> > +        }
> > +    }
> > +
> > +    free(filepattern);
> > +    
> > +    return r;
> > +}
> > +
> > +// Print to stdout wether <service> is enabled or disabled on boot
> > +// Prints <service> as Not available when initscript is not found
> > +// in either enabled_path or disabled_path.
> > +void print_boot_status(char *service) {
> > +    char *filepattern = malloc((3 + strlen(service) + 1) *
> > sizeof(char));
> > +    sprintf(filepattern, "S??%s", service);
> 
> See above.
> 
> > +    char *enabled = find_file_in_dir(enabled_path, filepattern);
> > +    char *disabled = find_file_in_dir(disabled_path, filepattern);
> > +
> > +    if (enabled != NULL) 
> > +        fprintf(stdout, "%s is enabled on boot.\n", service);
> > +    else if (disabled != NULL) 
> > +        fprintf(stdout, "%s is disabled on boot.\n", service);
> > +    else
> > +        fprintf(stdout, "%s is not available for boot. (Service
> > has no valid symlink in either %s or %s).\n", service,
> > enabled_path, disabled_path);
> > +
> > +    free(filepattern);
> > +}
> > +
> > int main(int argc, char *argv[]) {
> > -       char command[BUFFER_SIZE];
> > -
> > -       if (!(initsetuid()))
> > -               exit(1);
> > -
> > -       if (argc < 3) {
> > -               fprintf(stderr, "\nMissing arguments.\n\naddonctrl
> > addon (start|stop|restart|reload|enable|disable)\n\n");
> > -               exit(1);
> > -       }
> > -
> > -       const char* name = argv[1];
> > -
> > -       if (strlen(name) > 32) {
> > -           fprintf(stderr, "\nString to large.\n\naddonctrl addon
> > (start|stop|restart|reload|enable|disable)\n\n");
> > -           exit(1);
> > -       }
> > -
> > -       // Check if the input argument is valid
> > -       if (!is_valid_argument_alnum(name)) {
> > -               fprintf(stderr, "Invalid add-on name: %s\n", name);
> > -               exit(2);
> > -       }
> > -
> > -       sprintf(command, "/opt/pakfire/db/installed/meta-%s",
> > name);
> > -       FILE *fp = fopen(command,"r");
> > -       if ( fp ) {
> > -           fclose(fp);
> > -       } else {
> > -           fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl
> > addon (start|stop|restart|reload|status|enable|disable)\n\n",
> > name);
> > -           exit(1);
> > -       }
> > -
> > -       if (strcmp(argv[2], "start") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1,
> > "/etc/rc.d/init.d/%s start", name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "stop") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1,
> > "/etc/rc.d/init.d/%s stop", name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "restart") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1,
> > "/etc/rc.d/init.d/%s restart", name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "reload") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1,
> > "/etc/rc.d/init.d/%s reload", name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "status") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1,
> > "/etc/rc.d/init.d/%s status", name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "enable") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1, "mv -f
> > /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
> > -               safe_system(command);
> > -       } else if (strcmp(argv[2], "disable") == 0) {
> > -               snprintf(command, BUFFER_SIZE - 1, "mkdir -p
> > /etc/rc.d/rc3.d/off");
> > -               safe_system(command);
> > -               snprintf(command, BUFFER_SIZE - 1, "mv -f
> > /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
> > -               safe_system(command);
> > -       } else {
> > -               fprintf(stderr, "\nBad argument given.\n\naddonctrl
> > addon (start|stop|restart|reload|enable|disable)\n\n");
> > -               exit(1);
> > -       }
> > -
> > -       return 0;
> > +    char **services = NULL;
> > +    char **services_ptr = NULL;
> > +    int servicescnt = 0;
> > +    char *addon = argv[1];
> > +    char *action = argv[2];
> > +    char *service_filter = NULL;
> > +    int actioned = 0;
> > +    int r = 0;
> > +
> > +    if (!(initsetuid()))
> > +        exit(1);
> > +
> > +    if (argc < 3) {
> > +        fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage);
> > +        exit(1);
> > +    }
> > +
> > +    if (argc == 4) 
> > +        service_filter = argv[3];
> > +
> > +    if (strlen(addon) > 32) {
> > +        fprintf(stderr, "\nString to large.\n\n%s\n\n", usage);
> > +        exit(1);
> > +    }
> 
> How is 32 chosen?
> 

I have absolutely no idea. This comes straight from the original code.
I figured it would probably have a good reason, so I left it like this.

> > +
> > +    // Check if the input argument is valid
> > +    if (!is_valid_argument_alnum(addon)) {
> > +        fprintf(stderr, "Invalid add-on name: %s.\n", addon);
> > +        exit(2);
> > +    }
> > +
> > +    // Get initscript name(s) from addon metadata
> > +    services_ptr = get_addon_services(addon, &servicescnt);
> > +    services = services_ptr;
> > +    if (services == NULL || *services == 0) {
> > +        if (strcmp(errormsg, "") != 0)
> > +            fprintf(stderr, "\n%s\n\n", errormsg);
> > +        else
> > +            fprintf(stderr, "\nAddon '%s' has no services.\n\n",
> > addon);
> > +        exit(1);
> > +    }
> > +
> > +    if (strcmp(action, "start") == 0 ||
> > +        strcmp(action, "stop") == 0 ||
> > +        strcmp(action, "restart") == 0 ||
> > +        strcmp(action, "reload") == 0 ||
> > +        strcmp(action, "status") == 0) {
> > +
> > +        while (*services != 0) {
> > +            if ((service_filter != NULL && strcmp(service_filter,
> > *services) == 0) || 
> > +                 service_filter == NULL) {
> > +                if (initscript_action(*services, action) != 0) 
> > +                    r = 1;
> > +                ++actioned;
> 
> Wouldn’t it be a better ideal to pass the service_filter argument to
> the action function and only filter once in that function?
> 
> Otherwise you are copying the same code over and over again. Or have
> a helper function that will be called from initscript_action,
> toggle_service and so on?
> 
will investigate 

> > +            }
> > +            ++services;
> > +        }
> > +
> > +    } else if (strcmp(action, "enable") == 0 ||
> > +               strcmp(action, "disable") == 0) {
> > +
> > +        while (*services != 0) {
> > +            if ((service_filter != NULL && strcmp(service_filter,
> > *services) == 0) || 
> > +                 service_filter == NULL) {
> > +                if (toggle_service(*services, action) == 0) {
> > +                    fprintf(stdout, "%sd service %s\n", action,
> > *services);
> > +                }
> > +                else {
> > +                    r = 1;
> > +                    fprintf(stderr, "\n%s\n\n", errormsg);
> > +                }
> > +                
> > +                ++actioned;
> > +            }
> > +            ++services;
> > +        }
> > +
> > +    } else if (strcmp(action, "boot-status") == 0) {
> > +        while(*services != 0) {
> > +            if ((service_filter != NULL && strcmp(service_filter,
> > *services) == 0) || 
> > +                 service_filter == NULL) {
> > +                print_boot_status(*services);
> > +                ++actioned;
> > +            }
> > +            ++services;
> > +        }
> > +    
> > +    } else if (strcmp(action, "list-services") == 0) {
> > +        fprintf(stdout, "\nServices for addon %s:\n", addon);
> > +        while (*services != 0) {
> > +            fprintf(stdout, "  %s\n", *services);
> > +            ++actioned;
> > +            ++services;
> > +        }
> > +        fprintf(stdout, "\n");
> > +
> > +    } else {
> > +        fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage);
> > +        r = 1;
> > +    }
> > +
> > +    if (r == 0 && service_filter != NULL && actioned == 0) {
> > +        fprintf(stderr, "\nNo service %s found for addon %s. Use
> > 'list-services' to get a list of available services\n\n%s\n\n",
> > service_filter, addon, usage);
> > +        r = 1;
> > +    }
> > +
> > +    // Cleanup
> > +    for(int i = 0; i < servicescnt; i++) 
> > +        free(services_ptr[i]);
> > +    free(services_ptr);
> > +
> > +    return r;
> > }
> > -- 
> > 2.37.3
> 
> So all in all, this is the way to go. Since we have all this metadata
> now, we should use it, and this is a very good place to start.
> 
> However, there are some problems in this implementation and since
> this code is gaining root privileges, we need to make sure that it is
> 100% safe. Safe against any garbage inputs that might have ended up
> here, safe against some making some easy mistakes when the script is
> being changed again next time. So basically, do our best here. Sorry
> to be so strict, but the setuid binaries are a massive pain point in
> the current web UI and therefore we now have the great pleasure to
> fix old design issues.

I have no problem with you being strict on this, I wouldn't expect
otherwise as this is, like you said, of uttermost importance that it is
coded as safe as possible. And I'm also happy to (re)gain more
knowledge and insights in my rusty C skills.

So I'll try to address all your comments in a next version of this
patch as soon as possible.. or come back with more questions :-). 

Regards
Robin
> 
> -Michael
> 
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
>
  

Hi Michael


Robin Roevens schreef op di 04-10-2022 om 13:40 [+0200]:
> Hi Michael
> 
> Michael Tremer schreef op di 04-10-2022 om 11:28 [+0100]:
> > Hello Robin,
> > 
> > This is a little bit harder to review...
> I assumed so, as it is practically a full rewrite :-).
> for many of your comments, I will have to investigate and educate
> myself a bit more. I'll mark them when with 'will check' for now :-)
> 
> > 
> > > On 3 Oct 2022, at 16:27, Robin Roevens
> > > <robin.roevens@disroot.org>
> > > wrote:
> > > 
> > > * Addonctrl will now check in addon metadata for the exact
> > > initscript
> > >  names (Services). If more than one initscript is defined for an
> > > addon,
> > >  the requested action will be performed on all listed
> > > initscripts.
> > > * Added posibility to perform action on a specific initscript of
> > > an
> > >  addon instead of on all initscripts of the addon.
> > 
> > Is this actually used anywhere? I cannot come up with an example
> > where this would be relevant.
> Yes, this is the only way services.cgi would be able to
> start/stop/enable/disable single services instead of all services of
> an
> addon.
> As discussed with Adolf in the bug report, we wanted to give users
> fine
> control of which services to stop/start/enable/disable on service-
> level
> and not on addon-level. 
> When, for example a users wants a single service of a multi-service
> addon to be disabled, I think he should be able to. Also with addon
> libvirt which has 2 services libvirtd and virtlogd, if one of those 2
> hangs or starts acting weirdly a user should be able to restart that
> specific service.
> (this, for example, also opens up the possibility of adding a user
> optional suspend/resume guests initscript to that addon in the
> future)
> 
> > 
> > > * New action 'list-services' to display a list of services
> > > related
> > > to
> > >  an addon.
> > 
> > Agreed.
> > 
> > > * New action 'boot-status' to display wether service(s) are
> > > enabled
> > >  to start on boot or not.
> > 
> > Agreed.
> > 
> > > * More error checking and cleaner error reporting to user
> > 
> > I like this :)
> > 
> > > * General cleanup and code restructuring to avoid code
> > > duplication
> > > * Updated and made usage instructions more verbose.
> > > 
> > > Fixes: Bug#12935
> > > Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> > > ---
> > > src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++-
> > > --
> > > ---
> > > 1 file changed, 323 insertions(+), 61 deletions(-)
> > > 
> > > diff --git a/src/misc-progs/addonctrl.c b/src/misc-
> > > progs/addonctrl.c
> > > index 14b4b1325..277bd1809 100644
> > > --- a/src/misc-progs/addonctrl.c
> > > +++ b/src/misc-progs/addonctrl.c
> > > @@ -10,71 +10,333 @@
> > > #include <string.h>
> > > #include <unistd.h>
> > > #include <sys/types.h>
> > > +#include <sys/stat.h>
> > > #include <fcntl.h>
> > > +#include <dirent.h>
> > > +#include <errno.h>
> > > #include "setuid.h"
> > > 
> > > #define BUFFER_SIZE 1024
> > > 
> > > +const char *enabled_path = "/etc/rc.d/rc3.d";
> > > +const char *disabled_path = "/etc/rc.d/rc3.d/off";
> > > +
> > > +char errormsg[BUFFER_SIZE] = "";
> > > +char *usage = 
> > > +    "Usage\n"
> > > +    "  addonctrl <addon>
> > > (start|stop|restart|reload|enable|disable|status|boot-
> > > status|list-
> > > services) [<service>]\n"
> > > +    "\n"
> > > +    "Options:\n"
> > > +    "  <addon>\t\tName of the addon to control\n"
> > > +    "  <service>\t\tSpecific service of the addon to control
> > > (optional)\n"
> > > +    "  \t\t\tBy default the requested action is performed on all
> > > related services. See also 'list-services'.\n"
> > > +    "  start\t\t\tStart service(s) of the addon\n"
> > > +    "  stop\t\t\tStop service(s) of the addon\n"
> > > +    "  restart\t\tRestart service(s) of the addon\n"
> > > +    "  enable\t\tEnable service(s) of the addon to start at
> > > boot\n"
> > > +    "  disable\t\tDisable service(s) of the addon to start at
> > > boot\n"
> > > +    "  status\t\tDisplay current state of addon service(s)\n"
> > > +    "  boot-status\t\tDisplay wether service(s) is enabled on
> > > boot
> > > or not\n"
> > > +    "  list-services\t\tDisplay a list of services related to
> > > the
> > > addon";
> > 
> > This string could be const, but I am sure the compiler would assume
> > that anyways.
> 
> Probably, but I'll make it const manually.
> 
> > 
> > > +
> > > +// Check if <text> matches <pattern>. Wildcard '?' matches any
> > > single character.
> > > +// Returns 1 when found, 0 when not found
> > > +int match(const char *pattern, const char *text) {
> > > +    if (pattern[0] == '\0' && *text == '\0')
> > > +        return 1;
> > 
> > Do you not want to check whether you have reached the end of either
> > string?
> > 
> > I would have written it as:
> > 
> >   if (!*pattern || !*text)
> >     return 1;
> > 
> both have to be at their end at the same time, otherwise it should
> not
> be a match. If pattern is at the end but text is not, it would also
> match filenames that have additional characters after the matched
> part.
> So in the hypothetical case that there were 2 services with a mutual
> beginning of the filename: for example 'zabbix' and 'zabbix_agentd'.
> Matching for 'zabbix' would also match 'zabbix_agentd'.
> Not that there currently are such cases in all possible initscripts,
> I
> think. But a user could have added a custom initscript witch such
> name.
> 
> > 
> > > +    if (*text != '\0' && (pattern[0]=='?' || pattern[0]==*text))
> > > +        return match(pattern+1, text+1);
> > > +    return 0;
> > 
> > I have nothing against a recursive implementation, but potentially,
> > a
> > for loop would have been easier to read:
> > 
> > for (unsigned int i = 0; i < strlen(text); i++) {
> >         // ? matches anything
> >         if (pattern[i] == '?')
> >                 continue;
> > 
> >         // Check for mismatch
> >         if (pattern[i] != text[i])
> >                 return 1;
> > }
> > 
> > return 0;
> > 
> > This example does not include checking whether pattern is at least
> > as
> > long as text.
> > 
> > Since this function is only being used here, you could have
> > declared
> > it as static. The compiler might do this for you.
> > 
> > > +}
> > > +
> > > +// Find a file <path> using <filepattern> allowing the '?'
> > > wildcard.
> > > +// Returns the found filename or NULL if not found
> > > +char* find_file_in_dir(const char *path, const char
> > > *filepattern) 
> > > +{
> > > +    static struct dirent *entry;
> > > +    DIR *dp;
> > > +    int found = 0;
> > > +
> > > +    if ((dp = opendir(path)) != NULL) {
> > > +        while(found == 0 && (entry = readdir(dp)) != NULL)
> > > +            found = match(filepattern, entry->d_name);
> > > +
> > 
> > Now, reading through this, I understand what the match function is
> > used for :)
> > 
> > There is already a function for this:
> > 
> >   https://man7.org/linux/man-pages/man3/fnmatch.3.html
> > 
> > I think it works exactly the way you want this.
> 
> I was not aware of that function. As pointed out in the past and in
> the
> bug report; other than when I patched getipstat, my C coding skills
> have not been used for almost 25 year :-). I have been searching the
> web for a function like this but didn't find fnmach. I will try using
> that instead.
> 
> > 
> > > +        closedir(dp);
> > > +    }
> > > +
> > > +    if (! found) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return entry->d_name;
> > 
> > I am not sure where you can rely on closedir() not deallocating the
> > entry.
> > 
> > It would be best to change found to “char *” and call strdup(entry-
> > > d_name) after you found a match.
> > 
> > You can then simply return found at the end which should have been
> > initialised to NULL.
> 
> Agreed
> 
> > 
> > > +}
> > > +
> > > +// Reads Services metadata for <addon>.
> > > +// Returns pointer to array of strings containing the services
> > > for
> > > <addon> 
> > > +// and sets <servicescnt> to the number of found services
> > > +char **get_addon_services(const char *addon, int *servicescnt) {
> > > +    const char *metafile_prefix =
> > > "/opt/pakfire/db/installed/meta-
> > > ";
> > > +    const char *metadata_key = "Services";
> > > +    const char *keyvalue_delim = ":";
> > > +    const char *service_delim = " ";
> > > +    char *token;
> > > +    char **services = NULL;
> > > +    char *service;
> > > +    char line[BUFFER_SIZE];
> > > +    int i = 0;
> > 
> > Since you are passing “addon” to strlen(), I wouldn’t mind checking
> > that it isn’t NULL, like so:
> > 
> >   if (!addon) {
> >     errno = EINVAL;
> >     return NULL;
> >   }
> > 
> will do
> 
> > > +    char *metafile = malloc((strlen(metafile_prefix) +
> > > strlen(addon) + 1) * sizeof(char));
> > 
> > Why do you multiply this by the size of char? malloc() already
> > takes
> > bytes.
> 
> True, but I read a recommendation somewhere to explicitly do this for
> clarity. 
> Also I've read that char could be larger when UTF-8 is in play
> depending on the implementation of C used. Which is probably not the
> case here.
> 
> > 
> > You should also check whether you could successfully allocate the
> > memory and if not, return NULL.
> 
> agreed, better safe than sorry
> 
> > 
> > > + 
> > > +    sprintf(metafile, "%s%s", metafile_prefix, addon);
> > 
> > Alternatively, you can use asprintf() which will automatically
> > allocate a buffer of the correct size.
> 
> Will check 
> 
> > 
> > > +    FILE *fp = fopen(metafile,"r");
> > > +    if ( fp ) {
> > > +        // Get initscript(s) for addon from meta-file
> > > +        while (!feof(fp) && services == NULL) {
> > > +            if (fgets(line, BUFFER_SIZE - 1, fp) != NULL) {
> > > +                // Strip newline
> > > +                char *newline = strchr(line, '\n');
> > > +                if (newline) *newline = 0;
> > 
> > There is a getline() function which would combine this whole block
> > starting from fgets():
> > 
> >   https://man7.org/linux/man-pages/man3/getline.3.html
> > 
> > getline() will also automatically allocate a buffer of the correct
> > size for each line.
> 
> will check
I've read through the doc of getline, and I agree with it allocating a
buffer. But getline also explicitly includes the newline character
according to the doc. So I'd still have to remove it. Or did I miss
what you actually mean?
Or did you mean that I can merge the 'while' and the 'if(fgets..' lines
when using getline as it also implicitly checks for EOF.

> 
> > 
> > > +
> > > +                // Parse key/value and look for required key.
> > > +                token = strtok(line, keyvalue_delim);
> > > +                if (token != NULL && strcmp(token, metadata_key)
> > > == 0) {
> > > +                    token = strtok(NULL, keyvalue_delim);
> > > +                    if (token != NULL) {
> > > +                        services = malloc((strlen(token) + 1) *
> > > sizeof (char *));
> > 
> > Same as above. The multiplication is not required I believe.
> > 
> > > +                        // Put each service in services array
> > > +                        service = strtok(token, service_delim);
> > > +                        while (service != NULL) {
> > > +                            services[i] =
> > > malloc((strlen(service)
> > > + 1) * sizeof (char));
> > > +                            strcpy(services[i++], service);
> > 
> > Those two lines could just be replaced by using strdup().
> 
> Will check
> 
> > 
> > > +
> > > +                            service = strtok(NULL,
> > > service_delim);
> > > +                        }
> > 
> > I am not sure how you are allocating services. It is kind of like a
> > worst-case scenario, that you are doing here. I can live with this,
> > because this is a very short-lived program and wasting a couple of
> > extra bytes of memory is not going to be a disaster.
> > 
> > But you could also use reallocarray() in the inner while loop and
> > only allocate as much memory as you actually need:
> > 
> >   https://man.archlinux.org/man/reallocarray.3bsd.en
> 
> I have poured quite a bit of sweat trying to implement this :-). I
> will
> check reallocarray. I'm all for cleaner/better code.

I think I made a logical error in 
services = malloc((strlen(token) + 1) * sizeof (char *));

Somehow, I imagined all the strings, one after the other with a \0 in
between going into that memory.. But this is of course an array of
pointers (to pointers to strings) so it should just allocate '# of
services' * sizeof (char *).
It will indeed be able to hold all pointers this way, but the length of
the string will always be greater than the number of tokens I will get
from it so there is indeed always too much memory allocated.

Then of course I need to know the number of services beforehand, which
I don't, and it seems reallocarray solves this by enabling me to
dynamically extend the services-array pointer by pointer.


> 
> > 
> > > +                    }
> > > +                }
> > > +            } else {
> > > +                snprintf(errormsg, BUFFER_SIZE - 1, "Could not
> > > read '%s' metadata for addon '%s'.", metadata_key, addon);
> > > +            }
> > > +        }
> > > +        fclose(fp);
> > > +    }  else {
> > > +        snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not
> > > found.\n\n%s", addon, usage);
> > > +    }
> > > +
> > > +    free (metafile);
> > > +    *servicescnt = i;
> > > +    return services;
> > > +}
> > > +
> > > +// Calls initscript <service> with parameter <action>
> > > +int initscript_action(const char *service, const char *action) {
> > > +    const char *initd_path = "/etc/rc.d/init.d";
> > > +    char *command = malloc((strlen(initd_path) + 1 +
> > > strlen(service) + 1) * sizeof(char));
> > > +    int r = 0;
> > 
> > This function is opening us up for injecting any command from the
> > package metadata and run it as root.
> > 
> > If “service” for example is “samba; reboot”, then the initscript
> > that
> > you would want to launch is actually launched first and then the
> > following reboot command. This cannot stay that way.
> > 
> > You will need to use run() from setuid.c which is not launching a
> > shell and is therefore making this safer.
> > 
> > run() will take the initscript as first argument and any other
> > parameters as an array. So it could look like this:
> > 
> > int initscript_action(const char *service, const char *action) {
> >         char buffer[PATH_MAX];
> >         int r;
> > 
> >         r = snprintf(buffer, sizeof(buffer), "/etc/rc.d/init.d/%s",
> > service);
> >         if (r < 0)
> >                 return r;
> > 
> >         const char* argv[] = {
> >                 action,
> >                 NULL
> >         };
> > 
> >         return run(buffer, argv);
> > }
> > 
> > I know that you are checking inputs further down below, but I do
> > not
> > want to take the risk that something (even after some more changes
> > to
> > this file) is slipping through making us vulnerable that severely.
> 
> I more or less recycled the method used in the original code which
> also
> first created a command-string including executable and parameters
> and
> passing it to safe_system. Also not knowing about the run function.
> But run sounds indeed better and safer, so I'll go with that indeed.
> 
> > 
> > > +
> > > +    sprintf(command, "%s/%s %s", initd_path, service, action);
> > > +    r = safe_system(command);
> > > +    free(command);
> > > +
> > > +    return r; 
> > > +}
> > > +
> > > +// Move an initscript with filepattern from <src_path> to
> > > <dest_path>
> > > +// Returns:
> > > +//   -1: Error during move. Details in errno (returned by C
> > > rename
> > > function)
> > > +//   0: Success
> > > +//   1: file was not moved, but is already in <dest_path>
> > > +//   2: file does not exist in either in <src_path> or
> > > <dest_path>
> > > +int move_initscript_by_pattern(const char *src_path, const char
> > > *dest_path, const char *filepattern) {
> > > +    char *src, *dest;
> > > +    int r = 1;
> > > +    char *filename = find_file_in_dir(src_path, filepattern);
> > > +
> > > +    if ( filename != NULL ) {
> > > +        src = malloc((strlen(src_path) + 1 + strlen(filename) +
> > > 1)
> > > * sizeof(char));
> > > +        dest = malloc((strlen(dest_path) + 1 + strlen(filename)
> > > +
> > > 1) * sizeof(char));
> > > +        sprintf(src, "%s/%s", src_path, filename);
> > > +        sprintf(dest, "%s/%s", dest_path, filename);
> > 
> > See my example above how to allocate a string (or actually not do
> > that at all by using a stack variable).
> > 
> 
> will check

asprintf seems to do the job. I'm not sure how I should use a stack
variable for this, unless I would just 'char src[BUFFER_SIZE]' and use
snprint. But then every var I define like that wil take up 1Kb of
memory instead of the few bytes required.
I know, it's not that I use thousands of such vars, but if we try our
best to limit the services array size, I think we should also do this
here? Or did I misunderstand ?


> 
> > If allocation fails for some reason, your code will try to write to
> > a
> > NULL pointer and later free a NULL pointer.
> > 
> > > +
> > > +        r = rename(src, dest);
> > > +
> > > +        free(src);
> > > +        free(dest);
> > > +    } else {
> > > +        filename = find_file_in_dir(dest_path, filepattern);
> > > +        if (filename == NULL)
> > > +            r = 2;
> > > +    }
> > > +
> > > +    return r;
> > > +}
> > > +
> > > +// Enable/Disable addon service(s) by moving initscript symlink
> > > from/to disabled_path
> > > +int toggle_service(const char *service, const char *action) {
> > > +    const char *src_path, *dest_path;
> > > +    char *filepattern = malloc((3 + strlen(service) + 1) *
> > > sizeof(char));
> > > +    int r = 0;
> > > +    
> > > +    sprintf(filepattern, "S??%s", service);
> > 
> > See above.
> > 
> > > +
> > > +    if (strcmp(action, "enable") == 0) {
> > > +        src_path = disabled_path;
> > > +        dest_path = enabled_path;
> > > +    } else {
> > > +        src_path = enabled_path;
> > > +        dest_path = disabled_path;
> > > +    }
> > > +
> > > +    // Ensure disabled_path exists
> > > +    if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP +
> > > S_IROTH
> > > + S_IXOTH) == -1 && errno != EEXIST) {
> > > +        r = 1;
> > > +        snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s.
> > > (Error: %d)", disabled_path, errno);
> > > +    } else {
> > > +        r = move_initscript_by_pattern(src_path, dest_path,
> > > filepattern);
> > > +        if (r == -1 ) {
> > > +            r = 1;
> > > +            snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s
> > > %s.
> > > (Error: %d)", action, service, errno);
> > > +        } else if (r == 1) {
> > > +            snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is
> > > already %sd. Skipping...", service, action);
> > > +        } else if (r == 2) {
> > > +            snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s
> > > service %s. (Service has no valid symlink in %s).", action,
> > > service, src_path);
> > > +        }
> > > +    }
> > > +
> > > +    free(filepattern);
> > > +    
> > > +    return r;
> > > +}
> > > +
> > > +// Print to stdout wether <service> is enabled or disabled on
> > > boot
> > > +// Prints <service> as Not available when initscript is not
> > > found
> > > +// in either enabled_path or disabled_path.
> > > +void print_boot_status(char *service) {
> > > +    char *filepattern = malloc((3 + strlen(service) + 1) *
> > > sizeof(char));
> > > +    sprintf(filepattern, "S??%s", service);
> > 
> > See above.
> > 
> > > +    char *enabled = find_file_in_dir(enabled_path, filepattern);
> > > +    char *disabled = find_file_in_dir(disabled_path,
> > > filepattern);
> > > +
> > > +    if (enabled != NULL) 
> > > +        fprintf(stdout, "%s is enabled on boot.\n", service);
> > > +    else if (disabled != NULL) 
> > > +        fprintf(stdout, "%s is disabled on boot.\n", service);
> > > +    else
> > > +        fprintf(stdout, "%s is not available for boot. (Service
> > > has no valid symlink in either %s or %s).\n", service,
> > > enabled_path, disabled_path);
> > > +
> > > +    free(filepattern);
> > > +}
> > > +
> > > int main(int argc, char *argv[]) {
> > > -       char command[BUFFER_SIZE];
> > > -
> > > -       if (!(initsetuid()))
> > > -               exit(1);
> > > -
> > > -       if (argc < 3) {
> > > -               fprintf(stderr, "\nMissing
> > > arguments.\n\naddonctrl
> > > addon (start|stop|restart|reload|enable|disable)\n\n");
> > > -               exit(1);
> > > -       }
> > > -
> > > -       const char* name = argv[1];
> > > -
> > > -       if (strlen(name) > 32) {
> > > -           fprintf(stderr, "\nString to large.\n\naddonctrl
> > > addon
> > > (start|stop|restart|reload|enable|disable)\n\n");
> > > -           exit(1);
> > > -       }
> > > -
> > > -       // Check if the input argument is valid
> > > -       if (!is_valid_argument_alnum(name)) {
> > > -               fprintf(stderr, "Invalid add-on name: %s\n",
> > > name);
> > > -               exit(2);
> > > -       }
> > > -
> > > -       sprintf(command, "/opt/pakfire/db/installed/meta-%s",
> > > name);
> > > -       FILE *fp = fopen(command,"r");
> > > -       if ( fp ) {
> > > -           fclose(fp);
> > > -       } else {
> > > -           fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl
> > > addon (start|stop|restart|reload|status|enable|disable)\n\n",
> > > name);
> > > -           exit(1);
> > > -       }
> > > -
> > > -       if (strcmp(argv[2], "start") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1,
> > > "/etc/rc.d/init.d/%s start", name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "stop") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1,
> > > "/etc/rc.d/init.d/%s stop", name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "restart") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1,
> > > "/etc/rc.d/init.d/%s restart", name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "reload") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1,
> > > "/etc/rc.d/init.d/%s reload", name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "status") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1,
> > > "/etc/rc.d/init.d/%s status", name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "enable") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1, "mv -f
> > > /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
> > > -               safe_system(command);
> > > -       } else if (strcmp(argv[2], "disable") == 0) {
> > > -               snprintf(command, BUFFER_SIZE - 1, "mkdir -p
> > > /etc/rc.d/rc3.d/off");
> > > -               safe_system(command);
> > > -               snprintf(command, BUFFER_SIZE - 1, "mv -f
> > > /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
> > > -               safe_system(command);
> > > -       } else {
> > > -               fprintf(stderr, "\nBad argument
> > > given.\n\naddonctrl
> > > addon (start|stop|restart|reload|enable|disable)\n\n");
> > > -               exit(1);
> > > -       }
> > > -
> > > -       return 0;
> > > +    char **services = NULL;
> > > +    char **services_ptr = NULL;
> > > +    int servicescnt = 0;
> > > +    char *addon = argv[1];
> > > +    char *action = argv[2];
> > > +    char *service_filter = NULL;
> > > +    int actioned = 0;
> > > +    int r = 0;
> > > +
> > > +    if (!(initsetuid()))
> > > +        exit(1);
> > > +
> > > +    if (argc < 3) {
> > > +        fprintf(stderr, "\nMissing arguments.\n\n%s\n\n",
> > > usage);
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (argc == 4) 
> > > +        service_filter = argv[3];
> > > +
> > > +    if (strlen(addon) > 32) {
> > > +        fprintf(stderr, "\nString to large.\n\n%s\n\n", usage);
> > > +        exit(1);
> > > +    }
> > 
> > How is 32 chosen?
> > 
> 
> I have absolutely no idea. This comes straight from the original
> code.
> I figured it would probably have a good reason, so I left it like
> this.
> 
> > > +
> > > +    // Check if the input argument is valid
> > > +    if (!is_valid_argument_alnum(addon)) {
> > > +        fprintf(stderr, "Invalid add-on name: %s.\n", addon);
> > > +        exit(2);
> > > +    }
> > > +
> > > +    // Get initscript name(s) from addon metadata
> > > +    services_ptr = get_addon_services(addon, &servicescnt);
> > > +    services = services_ptr;
> > > +    if (services == NULL || *services == 0) {
> > > +        if (strcmp(errormsg, "") != 0)
> > > +            fprintf(stderr, "\n%s\n\n", errormsg);
> > > +        else
> > > +            fprintf(stderr, "\nAddon '%s' has no services.\n\n",
> > > addon);
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (strcmp(action, "start") == 0 ||
> > > +        strcmp(action, "stop") == 0 ||
> > > +        strcmp(action, "restart") == 0 ||
> > > +        strcmp(action, "reload") == 0 ||
> > > +        strcmp(action, "status") == 0) {
> > > +
> > > +        while (*services != 0) {
> > > +            if ((service_filter != NULL &&
> > > strcmp(service_filter,
> > > *services) == 0) || 
> > > +                 service_filter == NULL) {
> > > +                if (initscript_action(*services, action) != 0) 
> > > +                    r = 1;
> > > +                ++actioned;
> > 
> > Wouldn’t it be a better ideal to pass the service_filter argument
> > to
> > the action function and only filter once in that function?
> > 
> > Otherwise you are copying the same code over and over again. Or
> > have
> > a helper function that will be called from initscript_action,
> > toggle_service and so on?
> > 
> will investigate 

I'm not a fan of adding it to the action functions, as those functions
should just perform the requested action and otherwise not be called.
I can imagine calling a helper function where I somehow pass the
function to call when a service is actionable, but I'm not sure if I
can pass a function as argument to functions in C. And then it is maybe
getting a bit too complicated for what is trying to be achieved.

I could also put the while/if outside of the 'if(strcmp(action...'
block. But logically it then doesn't match in my head, as the loop will
then check for each service which action (start/stop/enable/...) to
perform while that action can and will never change in the loop.

Or I should see to set the services array to only the service defined
in the service_filter earlier on. So there won't be a need to loop over
all services when we know on which service to operate.

Now by typing all this. I think I'll go with the last solution as it
makes most sense in my head :-).

I should be able post a revision of this patchset by Thursday(night)

Regards
Robin
> 
> > > +            }
> > > +            ++services;
> > > +        }
> > > +
> > > +    } else if (strcmp(action, "enable") == 0 ||
> > > +               strcmp(action, "disable") == 0) {
> > > +
> > > +        while (*services != 0) {
> > > +            if ((service_filter != NULL &&
> > > strcmp(service_filter,
> > > *services) == 0) || 
> > > +                 service_filter == NULL) {
> > > +                if (toggle_service(*services, action) == 0) {
> > > +                    fprintf(stdout, "%sd service %s\n", action,
> > > *services);
> > > +                }
> > > +                else {
> > > +                    r = 1;
> > > +                    fprintf(stderr, "\n%s\n\n", errormsg);
> > > +                }
> > > +                
> > > +                ++actioned;
> > > +            }
> > > +            ++services;
> > > +        }
> > > +
> > > +    } else if (strcmp(action, "boot-status") == 0) {
> > > +        while(*services != 0) {
> > > +            if ((service_filter != NULL &&
> > > strcmp(service_filter,
> > > *services) == 0) || 
> > > +                 service_filter == NULL) {
> > > +                print_boot_status(*services);
> > > +                ++actioned;
> > > +            }
> > > +            ++services;
> > > +        }
> > > +    
> > > +    } else if (strcmp(action, "list-services") == 0) {
> > > +        fprintf(stdout, "\nServices for addon %s:\n", addon);
> > > +        while (*services != 0) {
> > > +            fprintf(stdout, "  %s\n", *services);
> > > +            ++actioned;
> > > +            ++services;
> > > +        }
> > > +        fprintf(stdout, "\n");
> > > +
> > > +    } else {
> > > +        fprintf(stderr, "\nBad argument given.\n\n%s\n\n",
> > > usage);
> > > +        r = 1;
> > > +    }
> > > +
> > > +    if (r == 0 && service_filter != NULL && actioned == 0) {
> > > +        fprintf(stderr, "\nNo service %s found for addon %s. Use
> > > 'list-services' to get a list of available services\n\n%s\n\n",
> > > service_filter, addon, usage);
> > > +        r = 1;
> > > +    }
> > > +
> > > +    // Cleanup
> > > +    for(int i = 0; i < servicescnt; i++) 
> > > +        free(services_ptr[i]);
> > > +    free(services_ptr);
> > > +
> > > +    return r;
> > > }
> > > -- 
> > > 2.37.3
> > 
> > So all in all, this is the way to go. Since we have all this
> > metadata
> > now, we should use it, and this is a very good place to start.
> > 
> > However, there are some problems in this implementation and since
> > this code is gaining root privileges, we need to make sure that it
> > is
> > 100% safe. Safe against any garbage inputs that might have ended up
> > here, safe against some making some easy mistakes when the script
> > is
> > being changed again next time. So basically, do our best here.
> > Sorry
> > to be so strict, but the setuid binaries are a massive pain point
> > in
> > the current web UI and therefore we now have the great pleasure to
> > fix old design issues.
> 
> I have no problem with you being strict on this, I wouldn't expect
> otherwise as this is, like you said, of uttermost importance that it
> is
> coded as safe as possible. And I'm also happy to (re)gain more
> knowledge and insights in my rusty C skills.
> 
> So I'll try to address all your comments in a next version of this
> patch as soon as possible.. or come back with more questions :-). 
> 
> Regards
> Robin
> > 
> > -Michael
> > 
> > > 
> > > 
> > > -- 
> > > Dit bericht is gescanned op virussen en andere gevaarlijke
> > > inhoud door MailScanner en lijkt schoon te zijn.
> > > 
> > 
> > 
>
  

Hello,

> On 5 Oct 2022, at 00:09, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi Michael
> 
> 
> Robin Roevens schreef op di 04-10-2022 om 13:40 [+0200]:
>> Hi Michael
>> 
>> Michael Tremer schreef op di 04-10-2022 om 11:28 [+0100]:
>>> Hello Robin,
>>> 
>>> This is a little bit harder to review...
>> I assumed so, as it is practically a full rewrite :-).

Yes, which is not bad given the code that you started with… That wasn’t good.

>> for many of your comments, I will have to investigate and educate
>> myself a bit more. I'll mark them when with 'will check' for now :-)

Okay. No problem…

I am a little bit late to reply to this since you already send a v2 of the patchset which I didn’t look at, yet…

To take things slow and thorough, I will reply to this first...

>> 
>>> 
>>>> On 3 Oct 2022, at 16:27, Robin Roevens
>>>> <robin.roevens@disroot.org>
>>>> wrote:
>>>> 
>>>> * Addonctrl will now check in addon metadata for the exact
>>>> initscript
>>>> names (Services). If more than one initscript is defined for an
>>>> addon,
>>>> the requested action will be performed on all listed
>>>> initscripts.
>>>> * Added posibility to perform action on a specific initscript of
>>>> an
>>>> addon instead of on all initscripts of the addon.
>>> 
>>> Is this actually used anywhere? I cannot come up with an example
>>> where this would be relevant.
>> Yes, this is the only way services.cgi would be able to
>> start/stop/enable/disable single services instead of all services of
>> an
>> addon.
>> As discussed with Adolf in the bug report, we wanted to give users
>> fine
>> control of which services to stop/start/enable/disable on service-
>> level
>> and not on addon-level. 
>> When, for example a users wants a single service of a multi-service
>> addon to be disabled, I think he should be able to. Also with addon
>> libvirt which has 2 services libvirtd and virtlogd, if one of those 2
>> hangs or starts acting weirdly a user should be able to restart that
>> specific service.
>> (this, for example, also opens up the possibility of adding a user
>> optional suspend/resume guests initscript to that addon in the
>> future)
>> 
>>> 
>>>> * New action 'list-services' to display a list of services
>>>> related
>>>> to
>>>> an addon.
>>> 
>>> Agreed.
>>> 
>>>> * New action 'boot-status' to display wether service(s) are
>>>> enabled
>>>> to start on boot or not.
>>> 
>>> Agreed.
>>> 
>>>> * More error checking and cleaner error reporting to user
>>> 
>>> I like this :)
>>> 
>>>> * General cleanup and code restructuring to avoid code
>>>> duplication
>>>> * Updated and made usage instructions more verbose.
>>>> 
>>>> Fixes: Bug#12935
>>>> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
>>>> ---
>>>> src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++-
>>>> --
>>>> ---
>>>> 1 file changed, 323 insertions(+), 61 deletions(-)
>>>> 
>>>> diff --git a/src/misc-progs/addonctrl.c b/src/misc-
>>>> progs/addonctrl.c
>>>> index 14b4b1325..277bd1809 100644
>>>> --- a/src/misc-progs/addonctrl.c
>>>> +++ b/src/misc-progs/addonctrl.c
>>>> @@ -10,71 +10,333 @@
>>>> #include <string.h>
>>>> #include <unistd.h>
>>>> #include <sys/types.h>
>>>> +#include <sys/stat.h>
>>>> #include <fcntl.h>
>>>> +#include <dirent.h>
>>>> +#include <errno.h>
>>>> #include "setuid.h"
>>>> 
>>>> #define BUFFER_SIZE 1024
>>>> 
>>>> +const char *enabled_path = "/etc/rc.d/rc3.d";
>>>> +const char *disabled_path = "/etc/rc.d/rc3.d/off";
>>>> +
>>>> +char errormsg[BUFFER_SIZE] = "";
>>>> +char *usage = 
>>>> +  "Usage\n"
>>>> +  "  addonctrl <addon>
>>>> (start|stop|restart|reload|enable|disable|status|boot-
>>>> status|list-
>>>> services) [<service>]\n"
>>>> +  "\n"
>>>> +  "Options:\n"
>>>> +  "  <addon>\t\tName of the addon to control\n"
>>>> +  "  <service>\t\tSpecific service of the addon to control
>>>> (optional)\n"
>>>> +  "  \t\t\tBy default the requested action is performed on all
>>>> related services. See also 'list-services'.\n"
>>>> +  "  start\t\t\tStart service(s) of the addon\n"
>>>> +  "  stop\t\t\tStop service(s) of the addon\n"
>>>> +  "  restart\t\tRestart service(s) of the addon\n"
>>>> +  "  enable\t\tEnable service(s) of the addon to start at
>>>> boot\n"
>>>> +  "  disable\t\tDisable service(s) of the addon to start at
>>>> boot\n"
>>>> +  "  status\t\tDisplay current state of addon service(s)\n"
>>>> +  "  boot-status\t\tDisplay wether service(s) is enabled on
>>>> boot
>>>> or not\n"
>>>> +  "  list-services\t\tDisplay a list of services related to
>>>> the
>>>> addon";
>>> 
>>> This string could be const, but I am sure the compiler would assume
>>> that anyways.
>> 
>> Probably, but I'll make it const manually.
>> 
>>> 
>>>> +
>>>> +// Check if <text> matches <pattern>. Wildcard '?' matches any
>>>> single character.
>>>> +// Returns 1 when found, 0 when not found
>>>> +int match(const char *pattern, const char *text) {
>>>> +  if (pattern[0] == '\0' && *text == '\0')
>>>> +  return 1;
>>> 
>>> Do you not want to check whether you have reached the end of either
>>> string?
>>> 
>>> I would have written it as:
>>> 
>>> if (!*pattern || !*text)
>>> return 1;
>>> 
>> both have to be at their end at the same time, otherwise it should
>> not
>> be a match. If pattern is at the end but text is not, it would also
>> match filenames that have additional characters after the matched
>> part.
>> So in the hypothetical case that there were 2 services with a mutual
>> beginning of the filename: for example 'zabbix' and 'zabbix_agentd'.
>> Matching for 'zabbix' would also match 'zabbix_agentd'.
>> Not that there currently are such cases in all possible initscripts,
>> I
>> think. But a user could have added a custom initscript witch such
>> name.
>> 
>>> 
>>>> +  if (*text != '\0' && (pattern[0]=='?' || pattern[0]==*text))
>>>> +  return match(pattern+1, text+1);
>>>> +  return 0;
>>> 
>>> I have nothing against a recursive implementation, but potentially,
>>> a
>>> for loop would have been easier to read:
>>> 
>>> for (unsigned int i = 0; i < strlen(text); i++) {
>>> // ? matches anything
>>> if (pattern[i] == '?')
>>> continue;
>>> 
>>> // Check for mismatch
>>> if (pattern[i] != text[i])
>>> return 1;
>>> }
>>> 
>>> return 0;
>>> 
>>> This example does not include checking whether pattern is at least
>>> as
>>> long as text.
>>> 
>>> Since this function is only being used here, you could have
>>> declared
>>> it as static. The compiler might do this for you.
>>> 
>>>> +}
>>>> +
>>>> +// Find a file <path> using <filepattern> allowing the '?'
>>>> wildcard.
>>>> +// Returns the found filename or NULL if not found
>>>> +char* find_file_in_dir(const char *path, const char
>>>> *filepattern) 
>>>> +{
>>>> +  static struct dirent *entry;
>>>> +  DIR *dp;
>>>> +  int found = 0;
>>>> +
>>>> +  if ((dp = opendir(path)) != NULL) {
>>>> +  while(found == 0 && (entry = readdir(dp)) != NULL)
>>>> +  found = match(filepattern, entry->d_name);
>>>> +
>>> 
>>> Now, reading through this, I understand what the match function is
>>> used for :)
>>> 
>>> There is already a function for this:
>>> 
>>> https://man7.org/linux/man-pages/man3/fnmatch.3.html
>>> 
>>> I think it works exactly the way you want this.
>> 
>> I was not aware of that function. As pointed out in the past and in
>> the
>> bug report; other than when I patched getipstat, my C coding skills
>> have not been used for almost 25 year :-). I have been searching the
>> web for a function like this but didn't find fnmach. I will try using
>> that instead.
>> 
>>> 
>>>> +  closedir(dp);
>>>> +  }
>>>> +
>>>> +  if (! found) {
>>>> +  return NULL;
>>>> +  }
>>>> +
>>>> +  return entry->d_name;
>>> 
>>> I am not sure where you can rely on closedir() not deallocating the
>>> entry.
>>> 
>>> It would be best to change found to “char *” and call strdup(entry-
>>>> d_name) after you found a match.
>>> 
>>> You can then simply return found at the end which should have been
>>> initialised to NULL.
>> 
>> Agreed
>> 
>>> 
>>>> +}
>>>> +
>>>> +// Reads Services metadata for <addon>.
>>>> +// Returns pointer to array of strings containing the services
>>>> for
>>>> <addon> 
>>>> +// and sets <servicescnt> to the number of found services
>>>> +char **get_addon_services(const char *addon, int *servicescnt) {
>>>> +  const char *metafile_prefix =
>>>> "/opt/pakfire/db/installed/meta-
>>>> ";
>>>> +  const char *metadata_key = "Services";
>>>> +  const char *keyvalue_delim = ":";
>>>> +  const char *service_delim = " ";
>>>> +  char *token;
>>>> +  char **services = NULL;
>>>> +  char *service;
>>>> +  char line[BUFFER_SIZE];
>>>> +  int i = 0;
>>> 
>>> Since you are passing “addon” to strlen(), I wouldn’t mind checking
>>> that it isn’t NULL, like so:
>>> 
>>> if (!addon) {
>>> errno = EINVAL;
>>> return NULL;
>>> }
>>> 
>> will do
>> 
>>>> +  char *metafile = malloc((strlen(metafile_prefix) +
>>>> strlen(addon) + 1) * sizeof(char));
>>> 
>>> Why do you multiply this by the size of char? malloc() already
>>> takes
>>> bytes.
>> 
>> True, but I read a recommendation somewhere to explicitly do this for
>> clarity. 
>> Also I've read that char could be larger when UTF-8 is in play
>> depending on the implementation of C used. Which is probably not the
>> case here.
>> 
>>> 
>>> You should also check whether you could successfully allocate the
>>> memory and if not, return NULL.
>> 
>> agreed, better safe than sorry
>> 
>>> 
>>>> + 
>>>> +  sprintf(metafile, "%s%s", metafile_prefix, addon);
>>> 
>>> Alternatively, you can use asprintf() which will automatically
>>> allocate a buffer of the correct size.
>> 
>> Will check 
>> 
>>> 
>>>> +  FILE *fp = fopen(metafile,"r");
>>>> +  if ( fp ) {
>>>> +  // Get initscript(s) for addon from meta-file
>>>> +  while (!feof(fp) && services == NULL) {
>>>> +  if (fgets(line, BUFFER_SIZE - 1, fp) != NULL) {
>>>> +  // Strip newline
>>>> +  char *newline = strchr(line, '\n');
>>>> +  if (newline) *newline = 0;
>>> 
>>> There is a getline() function which would combine this whole block
>>> starting from fgets():
>>> 
>>> https://man7.org/linux/man-pages/man3/getline.3.html
>>> 
>>> getline() will also automatically allocate a buffer of the correct
>>> size for each line.
>> 
>> will check
> I've read through the doc of getline, and I agree with it allocating a
> buffer. But getline also explicitly includes the newline character
> according to the doc. So I'd still have to remove it. Or did I miss
> what you actually mean?

Yes, it will read the entire line.

You can simply search for the end of the string and replace the last character with '\0’ to strip the newline.

Since you will need to copy the string anyways, you can also use strndup() and copy only strlen() - 1 characters.

> Or did you mean that I can merge the 'while' and the 'if(fgets..' lines
> when using getline as it also implicitly checks for EOF.

Here is a simple example where I used getline():

https://git.ipfire.org/?p=pakfire.git;a=blob;f=src/libpakfire/pakfire.c;hb=24622c9155b7bb21ebfd4ca6a27c9d0816b6826a#l609

The return value will tell you when you have reached the end of the file.

> 
>> 
>>> 
>>>> +
>>>> +  // Parse key/value and look for required key.
>>>> +  token = strtok(line, keyvalue_delim);
>>>> +  if (token != NULL && strcmp(token, metadata_key)
>>>> == 0) {
>>>> +  token = strtok(NULL, keyvalue_delim);
>>>> +  if (token != NULL) {
>>>> +  services = malloc((strlen(token) + 1) *
>>>> sizeof (char *));
>>> 
>>> Same as above. The multiplication is not required I believe.
>>> 
>>>> +  // Put each service in services array
>>>> +  service = strtok(token, service_delim);
>>>> +  while (service != NULL) {
>>>> +  services[i] =
>>>> malloc((strlen(service)
>>>> + 1) * sizeof (char));
>>>> +  strcpy(services[i++], service);
>>> 
>>> Those two lines could just be replaced by using strdup().
>> 
>> Will check
>> 
>>> 
>>>> +
>>>> +  service = strtok(NULL,
>>>> service_delim);
>>>> +  }
>>> 
>>> I am not sure how you are allocating services. It is kind of like a
>>> worst-case scenario, that you are doing here. I can live with this,
>>> because this is a very short-lived program and wasting a couple of
>>> extra bytes of memory is not going to be a disaster.
>>> 
>>> But you could also use reallocarray() in the inner while loop and
>>> only allocate as much memory as you actually need:
>>> 
>>> https://man.archlinux.org/man/reallocarray.3bsd.en
>> 
>> I have poured quite a bit of sweat trying to implement this :-). I
>> will
>> check reallocarray. I'm all for cleaner/better code.
> 
> I think I made a logical error in 
> services = malloc((strlen(token) + 1) * sizeof (char *));
> 
> Somehow, I imagined all the strings, one after the other with a \0 in
> between going into that memory.. But this is of course an array of
> pointers (to pointers to strings) so it should just allocate '# of
> services' * sizeof (char *).

You will have an array with pointers to some other memory that holds the string.

So the strings could be all over the place, but the pointers will be listed one after the other in memory.

> It will indeed be able to hold all pointers this way, but the length of
> the string will always be greater than the number of tokens I will get
> from it so there is indeed always too much memory allocated.

It is not strictly required to never ever allocate a byte too much - the allocator will probably allocate a whole page no matter what. But it is good to be as thorough as possible :)

Just hoping for the best will give you a really nice heap overflow.

> Then of course I need to know the number of services beforehand, which
> I don't, and it seems reallocarray solves this by enabling me to
> dynamically extend the services-array pointer by pointer.

An alternative could be to just allocate for lets say 128 services and stop parsing after you have used all slots in the array, but since reallocarray is kind of easy to use, I would prefer to avoid this.

> 
> 
>> 
>>> 
>>>> +  }
>>>> +  }
>>>> +  } else {
>>>> +  snprintf(errormsg, BUFFER_SIZE - 1, "Could not
>>>> read '%s' metadata for addon '%s'.", metadata_key, addon);
>>>> +  }
>>>> +  }
>>>> +  fclose(fp);
>>>> +  }  else {
>>>> +  snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not
>>>> found.\n\n%s", addon, usage);
>>>> +  }
>>>> +
>>>> +  free (metafile);
>>>> +  *servicescnt = i;
>>>> +  return services;
>>>> +}
>>>> +
>>>> +// Calls initscript <service> with parameter <action>
>>>> +int initscript_action(const char *service, const char *action) {
>>>> +  const char *initd_path = "/etc/rc.d/init.d";
>>>> +  char *command = malloc((strlen(initd_path) + 1 +
>>>> strlen(service) + 1) * sizeof(char));
>>>> +  int r = 0;
>>> 
>>> This function is opening us up for injecting any command from the
>>> package metadata and run it as root.
>>> 
>>> If “service” for example is “samba; reboot”, then the initscript
>>> that
>>> you would want to launch is actually launched first and then the
>>> following reboot command. This cannot stay that way.
>>> 
>>> You will need to use run() from setuid.c which is not launching a
>>> shell and is therefore making this safer.
>>> 
>>> run() will take the initscript as first argument and any other
>>> parameters as an array. So it could look like this:
>>> 
>>> int initscript_action(const char *service, const char *action) {
>>> char buffer[PATH_MAX];
>>> int r;
>>> 
>>> r = snprintf(buffer, sizeof(buffer), "/etc/rc.d/init.d/%s",
>>> service);
>>> if (r < 0)
>>> return r;
>>> 
>>> const char* argv[] = {
>>> action,
>>> NULL
>>> };
>>> 
>>> return run(buffer, argv);
>>> }
>>> 
>>> I know that you are checking inputs further down below, but I do
>>> not
>>> want to take the risk that something (even after some more changes
>>> to
>>> this file) is slipping through making us vulnerable that severely.
>> 
>> I more or less recycled the method used in the original code which
>> also
>> first created a command-string including executable and parameters
>> and
>> passing it to safe_system. Also not knowing about the run function.
>> But run sounds indeed better and safer, so I'll go with that indeed.

Let’s set a good example here and be as safe as possible again.

There are plenty of other checks that we rely on that run before this part of the code, but any future change in those might destroy any safe guard and give people a chance to run any commands as root from the web UI. A lot is at stake here.

>> 
>>> 
>>>> +
>>>> +  sprintf(command, "%s/%s %s", initd_path, service, action);
>>>> +  r = safe_system(command);
>>>> +  free(command);
>>>> +
>>>> +  return r; 
>>>> +}
>>>> +
>>>> +// Move an initscript with filepattern from <src_path> to
>>>> <dest_path>
>>>> +// Returns:
>>>> +//  -1: Error during move. Details in errno (returned by C
>>>> rename
>>>> function)
>>>> +//  0: Success
>>>> +//  1: file was not moved, but is already in <dest_path>
>>>> +//  2: file does not exist in either in <src_path> or
>>>> <dest_path>
>>>> +int move_initscript_by_pattern(const char *src_path, const char
>>>> *dest_path, const char *filepattern) {
>>>> +  char *src, *dest;
>>>> +  int r = 1;
>>>> +  char *filename = find_file_in_dir(src_path, filepattern);
>>>> +
>>>> +  if ( filename != NULL ) {
>>>> +  src = malloc((strlen(src_path) + 1 + strlen(filename) +
>>>> 1)
>>>> * sizeof(char));
>>>> +  dest = malloc((strlen(dest_path) + 1 + strlen(filename)
>>>> +
>>>> 1) * sizeof(char));
>>>> +  sprintf(src, "%s/%s", src_path, filename);
>>>> +  sprintf(dest, "%s/%s", dest_path, filename);
>>> 
>>> See my example above how to allocate a string (or actually not do
>>> that at all by using a stack variable).
>>> 
>> 
>> will check
> 
> asprintf seems to do the job. I'm not sure how I should use a stack
> variable for this, unless I would just 'char src[BUFFER_SIZE]' and use
> snprint. But then every var I define like that wil take up 1Kb of
> memory instead of the few bytes required.

Indeed, you would always over-allocate by a lot here. But it is on the stack, so it is not really a huge problem. It will only live in memory for as long as the function runs and will be wiped when you leave the function.

Alternatively, you could malloc() just as much memory as you need, and then make sure that you use free() at all the correct places, running a few times through the allocator and use a lot more CPU power to find a suitable slot of memory.

It would be a trade-off between how much CPU power you have, and how much memory you have to waste.

I tend to go more and more with wasting stack memory. I am not sure if that doesn’t have any unintended downsides, but it feels like any code written like that could be exploited much less (because heap buffer overflows are much worse) and runs substantially faster.

> I know, it's not that I use thousands of such vars, but if we try our
> best to limit the services array size, I think we should also do this
> here? Or did I misunderstand ?

I don’t seem to understand the question.

> 
> 
>> 
>>> If allocation fails for some reason, your code will try to write to
>>> a
>>> NULL pointer and later free a NULL pointer.
>>> 
>>>> +
>>>> +  r = rename(src, dest);
>>>> +
>>>> +  free(src);
>>>> +  free(dest);
>>>> +  } else {
>>>> +  filename = find_file_in_dir(dest_path, filepattern);
>>>> +  if (filename == NULL)
>>>> +  r = 2;
>>>> +  }
>>>> +
>>>> +  return r;
>>>> +}
>>>> +
>>>> +// Enable/Disable addon service(s) by moving initscript symlink
>>>> from/to disabled_path
>>>> +int toggle_service(const char *service, const char *action) {
>>>> +  const char *src_path, *dest_path;
>>>> +  char *filepattern = malloc((3 + strlen(service) + 1) *
>>>> sizeof(char));
>>>> +  int r = 0;
>>>> + 
>>>> +  sprintf(filepattern, "S??%s", service);
>>> 
>>> See above.
>>> 
>>>> +
>>>> +  if (strcmp(action, "enable") == 0) {
>>>> +  src_path = disabled_path;
>>>> +  dest_path = enabled_path;
>>>> +  } else {
>>>> +  src_path = enabled_path;
>>>> +  dest_path = disabled_path;
>>>> +  }
>>>> +
>>>> +  // Ensure disabled_path exists
>>>> +  if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP +
>>>> S_IROTH
>>>> + S_IXOTH) == -1 && errno != EEXIST) {
>>>> +  r = 1;
>>>> +  snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s.
>>>> (Error: %d)", disabled_path, errno);
>>>> +  } else {
>>>> +  r = move_initscript_by_pattern(src_path, dest_path,
>>>> filepattern);
>>>> +  if (r == -1 ) {
>>>> +  r = 1;
>>>> +  snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s
>>>> %s.
>>>> (Error: %d)", action, service, errno);
>>>> +  } else if (r == 1) {
>>>> +  snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is
>>>> already %sd. Skipping...", service, action);
>>>> +  } else if (r == 2) {
>>>> +  snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s
>>>> service %s. (Service has no valid symlink in %s).", action,
>>>> service, src_path);
>>>> +  }
>>>> +  }
>>>> +
>>>> +  free(filepattern);
>>>> + 
>>>> +  return r;
>>>> +}
>>>> +
>>>> +// Print to stdout wether <service> is enabled or disabled on
>>>> boot
>>>> +// Prints <service> as Not available when initscript is not
>>>> found
>>>> +// in either enabled_path or disabled_path.
>>>> +void print_boot_status(char *service) {
>>>> +  char *filepattern = malloc((3 + strlen(service) + 1) *
>>>> sizeof(char));
>>>> +  sprintf(filepattern, "S??%s", service);
>>> 
>>> See above.
>>> 
>>>> +  char *enabled = find_file_in_dir(enabled_path, filepattern);
>>>> +  char *disabled = find_file_in_dir(disabled_path,
>>>> filepattern);
>>>> +
>>>> +  if (enabled != NULL) 
>>>> +  fprintf(stdout, "%s is enabled on boot.\n", service);
>>>> +  else if (disabled != NULL) 
>>>> +  fprintf(stdout, "%s is disabled on boot.\n", service);
>>>> +  else
>>>> +  fprintf(stdout, "%s is not available for boot. (Service
>>>> has no valid symlink in either %s or %s).\n", service,
>>>> enabled_path, disabled_path);
>>>> +
>>>> +  free(filepattern);
>>>> +}
>>>> +
>>>> int main(int argc, char *argv[]) {
>>>> -  char command[BUFFER_SIZE];
>>>> -
>>>> -  if (!(initsetuid()))
>>>> -  exit(1);
>>>> -
>>>> -  if (argc < 3) {
>>>> -  fprintf(stderr, "\nMissing
>>>> arguments.\n\naddonctrl
>>>> addon (start|stop|restart|reload|enable|disable)\n\n");
>>>> -  exit(1);
>>>> -  }
>>>> -
>>>> -  const char* name = argv[1];
>>>> -
>>>> -  if (strlen(name) > 32) {
>>>> -  fprintf(stderr, "\nString to large.\n\naddonctrl
>>>> addon
>>>> (start|stop|restart|reload|enable|disable)\n\n");
>>>> -  exit(1);
>>>> -  }
>>>> -
>>>> -  // Check if the input argument is valid
>>>> -  if (!is_valid_argument_alnum(name)) {
>>>> -  fprintf(stderr, "Invalid add-on name: %s\n",
>>>> name);
>>>> -  exit(2);
>>>> -  }
>>>> -
>>>> -  sprintf(command, "/opt/pakfire/db/installed/meta-%s",
>>>> name);
>>>> -  FILE *fp = fopen(command,"r");
>>>> -  if ( fp ) {
>>>> -  fclose(fp);
>>>> -  } else {
>>>> -  fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl
>>>> addon (start|stop|restart|reload|status|enable|disable)\n\n",
>>>> name);
>>>> -  exit(1);
>>>> -  }
>>>> -
>>>> -  if (strcmp(argv[2], "start") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1,
>>>> "/etc/rc.d/init.d/%s start", name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "stop") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1,
>>>> "/etc/rc.d/init.d/%s stop", name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "restart") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1,
>>>> "/etc/rc.d/init.d/%s restart", name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "reload") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1,
>>>> "/etc/rc.d/init.d/%s reload", name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "status") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1,
>>>> "/etc/rc.d/init.d/%s status", name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "enable") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1, "mv -f
>>>> /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
>>>> -  safe_system(command);
>>>> -  } else if (strcmp(argv[2], "disable") == 0) {
>>>> -  snprintf(command, BUFFER_SIZE - 1, "mkdir -p
>>>> /etc/rc.d/rc3.d/off");
>>>> -  safe_system(command);
>>>> -  snprintf(command, BUFFER_SIZE - 1, "mv -f
>>>> /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
>>>> -  safe_system(command);
>>>> -  } else {
>>>> -  fprintf(stderr, "\nBad argument
>>>> given.\n\naddonctrl
>>>> addon (start|stop|restart|reload|enable|disable)\n\n");
>>>> -  exit(1);
>>>> -  }
>>>> -
>>>> -  return 0;
>>>> +  char **services = NULL;
>>>> +  char **services_ptr = NULL;
>>>> +  int servicescnt = 0;
>>>> +  char *addon = argv[1];
>>>> +  char *action = argv[2];
>>>> +  char *service_filter = NULL;
>>>> +  int actioned = 0;
>>>> +  int r = 0;
>>>> +
>>>> +  if (!(initsetuid()))
>>>> +  exit(1);
>>>> +
>>>> +  if (argc < 3) {
>>>> +  fprintf(stderr, "\nMissing arguments.\n\n%s\n\n",
>>>> usage);
>>>> +  exit(1);
>>>> +  }
>>>> +
>>>> +  if (argc == 4) 
>>>> +  service_filter = argv[3];
>>>> +
>>>> +  if (strlen(addon) > 32) {
>>>> +  fprintf(stderr, "\nString to large.\n\n%s\n\n", usage);
>>>> +  exit(1);
>>>> +  }
>>> 
>>> How is 32 chosen?
>>> 
>> 
>> I have absolutely no idea. This comes straight from the original
>> code.
>> I figured it would probably have a good reason, so I left it like
>> this.
>> 
>>>> +
>>>> +  // Check if the input argument is valid
>>>> +  if (!is_valid_argument_alnum(addon)) {
>>>> +  fprintf(stderr, "Invalid add-on name: %s.\n", addon);
>>>> +  exit(2);
>>>> +  }
>>>> +
>>>> +  // Get initscript name(s) from addon metadata
>>>> +  services_ptr = get_addon_services(addon, &servicescnt);
>>>> +  services = services_ptr;
>>>> +  if (services == NULL || *services == 0) {
>>>> +  if (strcmp(errormsg, "") != 0)
>>>> +  fprintf(stderr, "\n%s\n\n", errormsg);
>>>> +  else
>>>> +  fprintf(stderr, "\nAddon '%s' has no services.\n\n",
>>>> addon);
>>>> +  exit(1);
>>>> +  }
>>>> +
>>>> +  if (strcmp(action, "start") == 0 ||
>>>> +  strcmp(action, "stop") == 0 ||
>>>> +  strcmp(action, "restart") == 0 ||
>>>> +  strcmp(action, "reload") == 0 ||
>>>> +  strcmp(action, "status") == 0) {
>>>> +
>>>> +  while (*services != 0) {
>>>> +  if ((service_filter != NULL &&
>>>> strcmp(service_filter,
>>>> *services) == 0) || 
>>>> +  service_filter == NULL) {
>>>> +  if (initscript_action(*services, action) != 0) 
>>>> +  r = 1;
>>>> +  ++actioned;
>>> 
>>> Wouldn’t it be a better ideal to pass the service_filter argument
>>> to
>>> the action function and only filter once in that function?
>>> 
>>> Otherwise you are copying the same code over and over again. Or
>>> have
>>> a helper function that will be called from initscript_action,
>>> toggle_service and so on?
>>> 
>> will investigate 
> 
> I'm not a fan of adding it to the action functions, as those functions
> should just perform the requested action and otherwise not be called.
> I can imagine calling a helper function where I somehow pass the
> function to call when a service is actionable, but I'm not sure if I
> can pass a function as argument to functions in C. And then it is maybe
> getting a bit too complicated for what is trying to be achieved.

You could pass callbacks. I think that is what I had at the back of my mind.

Callbacks are a little bit complex, so I don’t have a problem if you want to avoid them.

Here is an example where I am using a callback:

  https://git.ipfire.org/?p=pakfire.git;a=blob;f=src/libpakfire/pwd.c;hb=24622c9155b7bb21ebfd4ca6a27c9d0816b6826a#l84

That is because the function pretty much does there same thing over and over, but just works on different values.

> I could also put the while/if outside of the 'if(strcmp(action...'
> block. But logically it then doesn't match in my head, as the loop will
> then check for each service which action (start/stop/enable/...) to
> perform while that action can and will never change in the loop.

I don’t think performance will be a problem here. It is indeed not the most efficient way, but if it generates easy to read/write/review code, then that is okay for me.

> Or I should see to set the services array to only the service defined
> in the service_filter earlier on. So there won't be a need to loop over
> all services when we know on which service to operate.
> 
> Now by typing all this. I think I'll go with the last solution as it
> makes most sense in my head :-).

I can live with either.

> I should be able post a revision of this patchset by Thursday(night)

Cool!

-Michael

> 
> Regards
> Robin
>> 
>>>> +  }
>>>> +  ++services;
>>>> +  }
>>>> +
>>>> +  } else if (strcmp(action, "enable") == 0 ||
>>>> +  strcmp(action, "disable") == 0) {
>>>> +
>>>> +  while (*services != 0) {
>>>> +  if ((service_filter != NULL &&
>>>> strcmp(service_filter,
>>>> *services) == 0) || 
>>>> +  service_filter == NULL) {
>>>> +  if (toggle_service(*services, action) == 0) {
>>>> +  fprintf(stdout, "%sd service %s\n", action,
>>>> *services);
>>>> +  }
>>>> +  else {
>>>> +  r = 1;
>>>> +  fprintf(stderr, "\n%s\n\n", errormsg);
>>>> +  }
>>>> + 
>>>> +  ++actioned;
>>>> +  }
>>>> +  ++services;
>>>> +  }
>>>> +
>>>> +  } else if (strcmp(action, "boot-status") == 0) {
>>>> +  while(*services != 0) {
>>>> +  if ((service_filter != NULL &&
>>>> strcmp(service_filter,
>>>> *services) == 0) || 
>>>> +  service_filter == NULL) {
>>>> +  print_boot_status(*services);
>>>> +  ++actioned;
>>>> +  }
>>>> +  ++services;
>>>> +  }
>>>> + 
>>>> +  } else if (strcmp(action, "list-services") == 0) {
>>>> +  fprintf(stdout, "\nServices for addon %s:\n", addon);
>>>> +  while (*services != 0) {
>>>> +  fprintf(stdout, "  %s\n", *services);
>>>> +  ++actioned;
>>>> +  ++services;
>>>> +  }
>>>> +  fprintf(stdout, "\n");
>>>> +
>>>> +  } else {
>>>> +  fprintf(stderr, "\nBad argument given.\n\n%s\n\n",
>>>> usage);
>>>> +  r = 1;
>>>> +  }
>>>> +
>>>> +  if (r == 0 && service_filter != NULL && actioned == 0) {
>>>> +  fprintf(stderr, "\nNo service %s found for addon %s. Use
>>>> 'list-services' to get a list of available services\n\n%s\n\n",
>>>> service_filter, addon, usage);
>>>> +  r = 1;
>>>> +  }
>>>> +
>>>> +  // Cleanup
>>>> +  for(int i = 0; i < servicescnt; i++) 
>>>> +  free(services_ptr[i]);
>>>> +  free(services_ptr);
>>>> +
>>>> +  return r;
>>>> }
>>>> -- 
>>>> 2.37.3
>>> 
>>> So all in all, this is the way to go. Since we have all this
>>> metadata
>>> now, we should use it, and this is a very good place to start.
>>> 
>>> However, there are some problems in this implementation and since
>>> this code is gaining root privileges, we need to make sure that it
>>> is
>>> 100% safe. Safe against any garbage inputs that might have ended up
>>> here, safe against some making some easy mistakes when the script
>>> is
>>> being changed again next time. So basically, do our best here.
>>> Sorry
>>> to be so strict, but the setuid binaries are a massive pain point
>>> in
>>> the current web UI and therefore we now have the great pleasure to
>>> fix old design issues.
>> 
>> I have no problem with you being strict on this, I wouldn't expect
>> otherwise as this is, like you said, of uttermost importance that it
>> is
>> coded as safe as possible. And I'm also happy to (re)gain more
>> knowledge and insights in my rusty C skills.
>> 
>> So I'll try to address all your comments in a next version of this
>> patch as soon as possible.. or come back with more questions :-). 
>> 
>> Regards
>> Robin
>>> 
>>> -Michael
>>> 
>>>> 
>>>> 
>>>> -- 
>>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>> 
>>> 
>>> 
>> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.