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

[Robin Roevens <robin.roevens@disroot.org>]

[-- Attachment #1: Type: text/plain, Size: 34202 bytes --]

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(a)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(a)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.
> > > 
> > 
> > 
> 

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