From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Roevens To: development@lists.ipfire.org Subject: Re: [PATCH 1/2] misc-progs: addonctrl: Add support for 'Services' metadata Date: Tue, 04 Oct 2022 13:40:33 +0200 Message-ID: In-Reply-To: <9C06AF20-31D0-4CEE-9779-2E4E9B1C64BC@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6524141316921330983==" List-Id: --===============6524141316921330983== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Michael Michael Tremer schreef op di 04-10-2022 om 11:28 [+0100]: > Hello Robin, >=20 > 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 :-) >=20 > > On 3 Oct 2022, at 16:27, Robin Roevens > > wrote: > >=20 > > * Addonctrl will now check in addon metadata for the exact > > initscript > > =C2=A0names (Services). If more than one initscript is defined for an > > addon, > > =C2=A0the requested action will be performed on all listed initscripts. > > * Added posibility to perform action on a specific initscript of an > > =C2=A0addon instead of on all initscripts of the addon. >=20 > 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.=C2=A0 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) >=20 > > * New action 'list-services' to display a list of services related > > to > > =C2=A0an addon. >=20 > Agreed. >=20 > > * New action 'boot-status' to display wether service(s) are enabled > > =C2=A0to start on boot or not. >=20 > Agreed. >=20 > > * More error checking and cleaner error reporting to user >=20 > I like this :) >=20 > > * General cleanup and code restructuring to avoid code duplication > > * Updated and made usage instructions more verbose. > >=20 > > Fixes: Bug#12935 > > Signed-off-by: Robin Roevens > > --- > > src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++--- > > --- > > 1 file changed, 323 insertions(+), 61 deletions(-) > >=20 > > 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" > >=20 > > #define BUFFER_SIZE 1024 > >=20 > > +const char *enabled_path =3D "/etc/rc.d/rc3.d"; > > +const char *disabled_path =3D "/etc/rc.d/rc3.d/off"; > > + > > +char errormsg[BUFFER_SIZE] =3D ""; > > +char *usage =3D=20 > > +=C2=A0=C2=A0=C2=A0 "Usage\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 addonctrl > > (start|stop|restart|reload|enable|disable|status|boot-status|list- > > services) []\n" > > +=C2=A0=C2=A0=C2=A0 "\n" > > +=C2=A0=C2=A0=C2=A0 "Options:\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 \t\tName of the addon to control\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 \t\tSpecific service of the addon to= control > > (optional)\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 \t\t\tBy default the requested action is perf= ormed on all > > related services. See also 'list-services'.\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 start\t\t\tStart service(s) of the addon\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 stop\t\t\tStop service(s) of the addon\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 restart\t\tRestart service(s) of the addon\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 enable\t\tEnable service(s) of the addon to s= tart at > > boot\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 disable\t\tDisable service(s) of the addon to= start at > > boot\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 status\t\tDisplay current state of addon serv= ice(s)\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 boot-status\t\tDisplay wether service(s) is e= nabled on boot > > or not\n" > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 list-services\t\tDisplay a list of services r= elated to the > > addon"; >=20 > This string could be const, but I am sure the compiler would assume > that anyways. Probably, but I'll make it const manually. >=20 > > + > > +// Check if matches . Wildcard '?' matches any > > single character. > > +// Returns 1 when found, 0 when not found > > +int match(const char *pattern, const char *text) { > > +=C2=A0=C2=A0=C2=A0 if (pattern[0] =3D=3D '\0' && *text =3D=3D '\0') > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 1; >=20 > Do you not want to check whether you have reached the end of either > string? >=20 > I would have written it as: >=20 > =C2=A0 if (!*pattern || !*text) > =C2=A0=C2=A0=C2=A0 return 1; >=20 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. >=20 > > +=C2=A0=C2=A0=C2=A0 if (*text !=3D '\0' && (pattern[0]=3D=3D'?' || patter= n[0]=3D=3D*text)) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return match(pattern+1, text+= 1); > > +=C2=A0=C2=A0=C2=A0 return 0; >=20 > I have nothing against a recursive implementation, but potentially, a > for loop would have been easier to read: >=20 > for (unsigned int i =3D 0; i < strlen(text); i++) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0// ? matches anything > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (pattern[i] =3D=3D '?') > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0continue; >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0// Check for mismatch > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (pattern[i] !=3D text[i]) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0return 1; > } >=20 > return 0; >=20 > This example does not include checking whether pattern is at least as > long as text. >=20 > Since this function is only being used here, you could have declared > it as static. The compiler might do this for you. >=20 > > +} > > + > > +// 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)=20 > > +{ > > +=C2=A0=C2=A0=C2=A0 static struct dirent *entry; > > +=C2=A0=C2=A0=C2=A0 DIR *dp; > > +=C2=A0=C2=A0=C2=A0 int found =3D 0; > > + > > +=C2=A0=C2=A0=C2=A0 if ((dp =3D opendir(path)) !=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while(found =3D=3D 0 && (entr= y =3D readdir(dp)) !=3D NULL) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 found= =3D match(filepattern, entry->d_name); > > + >=20 > Now, reading through this, I understand what the match function is > used for :) >=20 > There is already a function for this: >=20 > =C2=A0 https://man7.org/linux/man-pages/man3/fnmatch.3.html >=20 > 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. >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 closedir(dp); > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 if (! found) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return NULL; > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 return entry->d_name; >=20 > I am not sure where you can rely on closedir() not deallocating the > entry. >=20 > It would be best to change found to =E2=80=9Cchar *=E2=80=9D and call strdu= p(entry- > >d_name) after you found a match. >=20 > You can then simply return found at the end which should have been > initialised to NULL. Agreed >=20 > > +} > > + > > +// Reads Services metadata for . > > +// Returns pointer to array of strings containing the services for > > =20 > > +// and sets to the number of found services > > +char **get_addon_services(const char *addon, int *servicescnt) { > > +=C2=A0=C2=A0=C2=A0 const char *metafile_prefix =3D "/opt/pakfire/db/inst= alled/meta- > > "; > > +=C2=A0=C2=A0=C2=A0 const char *metadata_key =3D "Services"; > > +=C2=A0=C2=A0=C2=A0 const char *keyvalue_delim =3D ":"; > > +=C2=A0=C2=A0=C2=A0 const char *service_delim =3D " "; > > +=C2=A0=C2=A0=C2=A0 char *token; > > +=C2=A0=C2=A0=C2=A0 char **services =3D NULL; > > +=C2=A0=C2=A0=C2=A0 char *service; > > +=C2=A0=C2=A0=C2=A0 char line[BUFFER_SIZE]; > > +=C2=A0=C2=A0=C2=A0 int i =3D 0; >=20 > Since you are passing =E2=80=9Caddon=E2=80=9D to strlen(), I wouldn=E2=80= =99t mind checking > that it isn=E2=80=99t NULL, like so: >=20 > =C2=A0 if (!addon) { > =C2=A0=C2=A0=C2=A0 errno =3D EINVAL; > =C2=A0=C2=A0=C2=A0 return NULL; > =C2=A0 } >=20 will do > > +=C2=A0=C2=A0=C2=A0 char *metafile =3D malloc((strlen(metafile_prefix) + > > strlen(addon) + 1) * sizeof(char)); >=20 > 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.=C2=A0 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. >=20 > You should also check whether you could successfully allocate the > memory and if not, return NULL. agreed, better safe than sorry >=20 > > +=20 > > +=C2=A0=C2=A0=C2=A0 sprintf(metafile, "%s%s", metafile_prefix, addon); >=20 > Alternatively, you can use asprintf() which will automatically > allocate a buffer of the correct size. Will check=20 >=20 > > +=C2=A0=C2=A0=C2=A0 FILE *fp =3D fopen(metafile,"r"); > > +=C2=A0=C2=A0=C2=A0 if ( fp ) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Get initscript(s) for addo= n from meta-file > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (!feof(fp) && services = =3D=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (f= gets(line, BUFFER_SIZE - 1, fp) !=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 // Strip newline > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 char *newline =3D strchr(line, '\n'); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (newline) *newline =3D 0; >=20 > There is a getline() function which would combine this whole block > starting from fgets(): >=20 > =C2=A0 https://man7.org/linux/man-pages/man3/getline.3.html >=20 > getline() will also automatically allocate a buffer of the correct > size for each line. will check >=20 > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 // Parse key/value and look for required key. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 token =3D strtok(line, keyvalue_delim); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (token !=3D NULL && strcmp(token, metadata_key) > > =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 token =3D strtok(NULL, keyvalue_de= lim); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (token !=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 services = =3D malloc((strlen(token) + 1) * > > sizeof (char *)); >=20 > Same as above. The multiplication is not required I believe. >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Put eac= h service in services array > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 service = =3D strtok(token, service_delim); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (ser= vice !=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 services[i] =3D malloc((strlen(service) > > + 1) * sizeof (char)); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 strcpy(services[i++], service); >=20 > Those two lines could just be replaced by using strdup(). Will check >=20 > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 service =3D strtok(NULL, service_delim); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >=20 > 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. >=20 > But you could also use reallocarray() in the inner while loop and > only allocate as much memory as you actually need: >=20 > =C2=A0 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. >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } els= e { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 snprintf(errormsg, BUFFER_SIZE - 1, "Could not > > read '%s' metadata for addon '%s'.", metadata_key, addon); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fclose(fp); > > +=C2=A0=C2=A0=C2=A0 }=C2=A0 else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 snprintf(errormsg, BUFFER_SIZ= E - 1, "Addon '%s' not > > found.\n\n%s", addon, usage); > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 free (metafile); > > +=C2=A0=C2=A0=C2=A0 *servicescnt =3D i; > > +=C2=A0=C2=A0=C2=A0 return services; > > +} > > + > > +// Calls initscript with parameter > > +int initscript_action(const char *service, const char *action) { > > +=C2=A0=C2=A0=C2=A0 const char *initd_path =3D "/etc/rc.d/init.d"; > > +=C2=A0=C2=A0=C2=A0 char *command =3D malloc((strlen(initd_path) + 1 + > > strlen(service) + 1) * sizeof(char)); > > +=C2=A0=C2=A0=C2=A0 int r =3D 0; >=20 > This function is opening us up for injecting any command from the > package metadata and run it as root. >=20 > If =E2=80=9Cservice=E2=80=9D for example is =E2=80=9Csamba; reboot=E2=80=9D= , then the initscript that > you would want to launch is actually launched first and then the > following reboot command. This cannot stay that way. >=20 > You will need to use run() from setuid.c which is not launching a > shell and is therefore making this safer. >=20 > run() will take the initscript as first argument and any other > parameters as an array. So it could look like this: >=20 > int initscript_action(const char *service, const char *action) { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0char buffer[PATH_MAX]; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int r; >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0r =3D snprintf(buffer, size= of(buffer), "/etc/rc.d/init.d/%s", > service); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (r < 0) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0return r; >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const char* argv[] =3D { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0action, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0NULL > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}; >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return run(buffer, argv); > } >=20 > 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. >=20 > > + > > +=C2=A0=C2=A0=C2=A0 sprintf(command, "%s/%s %s", initd_path, service, act= ion); > > +=C2=A0=C2=A0=C2=A0 r =3D safe_system(command); > > +=C2=A0=C2=A0=C2=A0 free(command); > > + > > +=C2=A0=C2=A0=C2=A0 return r;=20 > > +} > > + > > +// Move an initscript with filepattern from to > > > > +// Returns: > > +//=C2=A0=C2=A0 -1: Error during move. Details in errno (returned by C re= name > > function) > > +//=C2=A0=C2=A0 0: Success > > +//=C2=A0=C2=A0 1: file was not moved, but is already in > > +//=C2=A0=C2=A0 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) { > > +=C2=A0=C2=A0=C2=A0 char *src, *dest; > > +=C2=A0=C2=A0=C2=A0 int r =3D 1; > > +=C2=A0=C2=A0=C2=A0 char *filename =3D find_file_in_dir(src_path, filepat= tern); > > + > > +=C2=A0=C2=A0=C2=A0 if ( filename !=3D NULL ) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 src =3D malloc((strlen(src_pa= th) + 1 + strlen(filename) + 1) > > * sizeof(char)); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dest =3D malloc((strlen(dest_= path) + 1 + strlen(filename) + > > 1) * sizeof(char)); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sprintf(src, "%s/%s", src_pat= h, filename); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sprintf(dest, "%s/%s", dest_p= ath, filename); >=20 > See my example above how to allocate a string (or actually not do > that at all by using a stack variable). >=20 will check > If allocation fails for some reason, your code will try to write to a > NULL pointer and later free a NULL pointer. >=20 > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D rename(src, dest); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free(src); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free(dest); > > +=C2=A0=C2=A0=C2=A0 } else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 filename =3D find_file_in_dir= (dest_path, filepattern); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (filename =3D=3D NULL) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D= 2; > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 return r; > > +} > > + > > +// Enable/Disable addon service(s) by moving initscript symlink > > from/to disabled_path > > +int toggle_service(const char *service, const char *action) { > > +=C2=A0=C2=A0=C2=A0 const char *src_path, *dest_path; > > +=C2=A0=C2=A0=C2=A0 char *filepattern =3D malloc((3 + strlen(service) + 1= ) * > > sizeof(char)); > > +=C2=A0=C2=A0=C2=A0 int r =3D 0; > > +=C2=A0=C2=A0=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 sprintf(filepattern, "S??%s", service); >=20 > See above. >=20 > > + > > +=C2=A0=C2=A0=C2=A0 if (strcmp(action, "enable") =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 src_path =3D disabled_path; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dest_path =3D enabled_path; > > +=C2=A0=C2=A0=C2=A0 } else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 src_path =3D enabled_path; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dest_path =3D disabled_path; > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 // Ensure disabled_path exists > > +=C2=A0=C2=A0=C2=A0 if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP = + S_IROTH > > + S_IXOTH) =3D=3D -1 && errno !=3D EEXIST) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D 1; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 snprintf(errormsg, BUFFER_SIZ= E -1, "Error creating %s. > > (Error: %d)", disabled_path, errno); > > +=C2=A0=C2=A0=C2=A0 } else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D move_initscript_by_patt= ern(src_path, dest_path, > > filepattern); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (r =3D=3D -1 ) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D= 1; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 snpri= ntf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. > > (Error: %d)", action, service, errno); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else if (r =3D=3D 1) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 snpri= ntf(errormsg, BUFFER_SIZE - 1, "Service %s is > > already %sd. Skipping...", service, action); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } else if (r =3D=3D 2) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 snpri= ntf(errormsg, BUFFER_SIZE - 1, "Unable to %s > > service %s. (Service has no valid symlink in %s).", action, > > service, src_path); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 free(filepattern); > > +=C2=A0=C2=A0=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 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) { > > +=C2=A0=C2=A0=C2=A0 char *filepattern =3D malloc((3 + strlen(service) + 1= ) * > > sizeof(char)); > > +=C2=A0=C2=A0=C2=A0 sprintf(filepattern, "S??%s", service); >=20 > See above. >=20 > > +=C2=A0=C2=A0=C2=A0 char *enabled =3D find_file_in_dir(enabled_path, file= pattern); > > +=C2=A0=C2=A0=C2=A0 char *disabled =3D find_file_in_dir(disabled_path, fi= lepattern); > > + > > +=C2=A0=C2=A0=C2=A0 if (enabled !=3D NULL)=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "%s is enable= d on boot.\n", service); > > +=C2=A0=C2=A0=C2=A0 else if (disabled !=3D NULL)=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "%s is disabl= ed on boot.\n", service); > > +=C2=A0=C2=A0=C2=A0 else > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "%s is not av= ailable for boot. (Service > > has no valid symlink in either %s or %s).\n", service, > > enabled_path, disabled_path); > > + > > +=C2=A0=C2=A0=C2=A0 free(filepattern); > > +} > > + > > int main(int argc, char *argv[]) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0char command[BUFFER_SIZE]; > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!(initsetuid())) > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0exit(1); > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (argc < 3) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0fprintf(stderr, "\nMissing arguments.\n\naddonctrl > > addon (start|stop|restart|reload|enable|disable)\n\n"); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0exit(1); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const char* name =3D argv[1]; > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (strlen(name) > 32) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(std= err, "\nString to large.\n\naddonctrl addon > > (start|stop|restart|reload|enable|disable)\n\n"); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0// Check if the input argument= is valid > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!is_valid_argument_alnum(n= ame)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0fprintf(stderr, "Invalid add-on name: %s\n", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0exit(2); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sprintf(command, "/opt/pakfire= /db/installed/meta-%s", > > name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0FILE *fp =3D fopen(command,"r"= ); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( fp ) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fclose(fp); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(std= err, "\nAddon '%s' not found.\n\naddonctrl > > addon (start|stop|restart|reload|status|enable|disable)\n\n", > > name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (strcmp(argv[2], "start") = =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, > > "/etc/rc.d/init.d/%s start", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "st= op") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, > > "/etc/rc.d/init.d/%s stop", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "re= start") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, > > "/etc/rc.d/init.d/%s restart", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "re= load") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, > > "/etc/rc.d/init.d/%s reload", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "st= atus") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, > > "/etc/rc.d/init.d/%s status", name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "en= able") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, "mv -f > > /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else if (strcmp(argv[2], "di= sable") =3D=3D 0) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, "mkdir -p > > /etc/rc.d/rc3.d/off"); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0snprintf(command, BUFFER_SIZE - 1, "mv -f > > /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0safe_system(command); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} else { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0fprintf(stderr, "\nBad argument given.\n\naddonctrl > > addon (start|stop|restart|reload|enable|disable)\n\n"); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0exit(1); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > - > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0; > > +=C2=A0=C2=A0=C2=A0 char **services =3D NULL; > > +=C2=A0=C2=A0=C2=A0 char **services_ptr =3D NULL; > > +=C2=A0=C2=A0=C2=A0 int servicescnt =3D 0; > > +=C2=A0=C2=A0=C2=A0 char *addon =3D argv[1]; > > +=C2=A0=C2=A0=C2=A0 char *action =3D argv[2]; > > +=C2=A0=C2=A0=C2=A0 char *service_filter =3D NULL; > > +=C2=A0=C2=A0=C2=A0 int actioned =3D 0; > > +=C2=A0=C2=A0=C2=A0 int r =3D 0; > > + > > +=C2=A0=C2=A0=C2=A0 if (!(initsetuid())) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > + > > +=C2=A0=C2=A0=C2=A0 if (argc < 3) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "\nMissing ar= guments.\n\n%s\n\n", usage); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 if (argc =3D=3D 4)=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 service_filter =3D argv[3]; > > + > > +=C2=A0=C2=A0=C2=A0 if (strlen(addon) > 32) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "\nString to = large.\n\n%s\n\n", usage); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > +=C2=A0=C2=A0=C2=A0 } >=20 > How is 32 chosen? >=20 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. > > + > > +=C2=A0=C2=A0=C2=A0 // Check if the input argument is valid > > +=C2=A0=C2=A0=C2=A0 if (!is_valid_argument_alnum(addon)) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "Invalid add-= on name: %s.\n", addon); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(2); > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 // Get initscript name(s) from addon metadata > > +=C2=A0=C2=A0=C2=A0 services_ptr =3D get_addon_services(addon, &servicesc= nt); > > +=C2=A0=C2=A0=C2=A0 services =3D services_ptr; > > +=C2=A0=C2=A0=C2=A0 if (services =3D=3D NULL || *services =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (strcmp(errormsg, "") !=3D= 0) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprin= tf(stderr, "\n%s\n\n", errormsg); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprin= tf(stderr, "\nAddon '%s' has no services.\n\n", > > addon); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 exit(1); > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 if (strcmp(action, "start") =3D=3D 0 || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strcmp(action, "stop") =3D=3D= 0 || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strcmp(action, "restart") =3D= =3D 0 || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strcmp(action, "reload") =3D= =3D 0 || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strcmp(action, "status") =3D= =3D 0) { > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (*services !=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((= service_filter !=3D NULL && strcmp(service_filter, > > *services) =3D=3D 0) ||=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 service_filter =3D=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (initscript_action(*services, action) !=3D 0)=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D 1; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 ++actioned; >=20 > Wouldn=E2=80=99t it be a better ideal to pass the service_filter argument to > the action function and only filter once in that function? >=20 > 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? >=20 will investigate=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++ser= vices; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 } else if (strcmp(action, "enable") =3D=3D 0 || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 strcmp(action, "disable") =3D=3D 0) { > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (*services !=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((= service_filter !=3D NULL && strcmp(service_filter, > > *services) =3D=3D 0) ||=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 service_filter =3D=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 if (toggle_service(*services, action) =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "%sd service %s\n"= , action, > > *services); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D 1; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "\n%s\n\n", errorm= sg); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 ++actioned; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++ser= vices; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 } else if (strcmp(action, "boot-status") =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while(*services !=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((= service_filter !=3D NULL && strcmp(service_filter, > > *services) =3D=3D 0) ||=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 service_filter =3D=3D NULL) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 print_boot_status(*services); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 ++actioned; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++ser= vices; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 } else if (strcmp(action, "list-services") =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "\nServices f= or addon %s:\n", addon); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (*services !=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprin= tf(stdout, "=C2=A0 %s\n", *services); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++act= ioned; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++ser= vices; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stdout, "\n"); > > + > > +=C2=A0=C2=A0=C2=A0 } else { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "\nBad argume= nt given.\n\n%s\n\n", usage); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D 1; > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 if (r =3D=3D 0 && service_filter !=3D NULL && actione= d =3D=3D 0) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r =3D 1; > > +=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 // Cleanup > > +=C2=A0=C2=A0=C2=A0 for(int i =3D 0; i < servicescnt; i++)=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free(services_ptr[i]); > > +=C2=A0=C2=A0=C2=A0 free(services_ptr); > > + > > +=C2=A0=C2=A0=C2=A0 return r; > > } > > --=20 > > 2.37.3 >=20 > 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. >=20 > 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 :-).=20 Regards Robin >=20 > -Michael >=20 > >=20 > >=20 > > --=20 > > Dit bericht is gescanned op virussen en andere gevaarlijke > > inhoud door MailScanner en lijkt schoon te zijn. > >=20 >=20 >=20 --=20 Dit bericht is gescanned op virussen en andere gevaarlijke inhoud door MailScanner en lijkt schoon te zijn. --===============6524141316921330983==--