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 > > > > > > 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 > > > --- > > > 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 > > > #include > > > #include > > > +#include > > > #include > > > +#include > > > +#include > > > #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 > > > (start|stop|restart|reload|enable|disable|status|boot- > > > status|list- > > > services) []\n" > > > +    "\n" > > > +    "Options:\n" > > > +    "  \t\tName of the addon to control\n" > > > +    "  \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 matches . 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 using 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 . > > > +// Returns pointer to array of strings containing the services > > > for > > > > > > +// and sets 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 with parameter > > > +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 to > > > > > > +// Returns: > > > +//   -1: Error during move. Details in errno (returned by C > > > rename > > > function) > > > +//   0: Success > > > +//   1: file was not moved, but is already in > > > +//   2: file does not exist in either in or > > > > > > +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 is enabled or disabled on > > > boot > > > +// Prints 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. > > > > > > > > -- Dit bericht is gescanned op virussen en andere gevaarlijke inhoud door MailScanner en lijkt schoon te zijn.