From mboxrd@z Thu Jan  1 00:00:00 1970
From: Robin Roevens <robin.roevens@disroot.org>
To: development@lists.ipfire.org
Subject:
 Re: [PATCH 1/2] misc-progs: addonctrl: Add support for 'Services' metadata
Date: Wed, 05 Oct 2022 01:09:55 +0200
Message-ID: <e61781be449726bbea580eb4d8833ca1d9de73b9.camel@sicho.home>
In-Reply-To: <ceba0d40f7f0c0a27122d6509287589927517ac6.camel@sicho.home>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2264331725858375788=="
List-Id: <development.lists.ipfire.org>

--===============2264331725858375788==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Michael


Robin Roevens schreef op di 04-10-2022 om 13:40 [+0200]:
> Hi Michael
>=20
> 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
> >=20
> > > On 3 Oct 2022, at 16:27, Robin Roevens
> > > <robin.roevens(a)disroot.org>
> > > 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
> >=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 <robin.roevens(a)disroot.org>
> > > ---
> > > 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 <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"
> > >=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 <addon>
> > > (start|stop|restart|reload|enable|disable|status|boot-
> > > status|list-
> > > services) [<service>]\n"
> > > +=C2=A0=C2=A0=C2=A0 "\n"
> > > +=C2=A0=C2=A0=C2=A0 "Options:\n"
> > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 <addon>\t\tName of the addon to control\n"
> > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 <service>\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 pe=
rformed 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=
 start 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 se=
rvice(s)\n"
> > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 boot-status\t\tDisplay wether service(s) is=
 enabled on
> > > boot
> > > or not\n"
> > > +=C2=A0=C2=A0=C2=A0 "=C2=A0 list-services\t\tDisplay a list of services=
 related to
> > > the
> > > addon";
> >=20
> > This string could be const, but I am sure the compiler would assume
> > that anyways.
>=20
> Probably, but I'll make it const manually.
>=20
> >=20
> > > +
> > > +// 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) {
> > > +=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
> >=20
> > > +=C2=A0=C2=A0=C2=A0 if (*text !=3D '\0' && (pattern[0]=3D=3D'?' || patt=
ern[0]=3D=3D*text))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return match(pattern+1, tex=
t+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 <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)=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 && (en=
try =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 fou=
nd =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.
>=20
> 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
> >=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 str=
dup(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.
>=20
> Agreed
>=20
> >=20
> > > +}
> > > +
> > > +// Reads Services metadata for <addon>.
> > > +// Returns pointer to array of strings containing the services
> > > for
> > > <addon>=20
> > > +// and sets <servicescnt> 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/installed/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
>=20
> > > +=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.
>=20
> 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
> >=20
> > You should also check whether you could successfully allocate the
> > memory and if not, return NULL.
>=20
> agreed, better safe than sorry
>=20
> >=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.
>=20
> Will check=20
>=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 ad=
don from meta-file
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (!feof(fp) && service=
s =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 =
(fgets(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.
>=20
> 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.

>=20
> >=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=
_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 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 service=
s =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 =
each 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 (=
service !=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().
>=20
> Will check
>=20
> >=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
>=20
> 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=20
services =3D 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.


>=20
> >=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 } e=
lse {
> > > +=C2=A0=C2=A0=C2=A0=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_S=
IZE - 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 <service> with parameter <action>
> > > +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, si=
zeof(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.
>=20
> 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
> >=20
> > > +
> > > +=C2=A0=C2=A0=C2=A0 sprintf(command, "%s/%s %s", initd_path, service, a=
ction);
> > > +=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 <src_path> to
> > > <dest_path>
> > > +// Returns:
> > > +//=C2=A0=C2=A0 -1: Error during move. Details in errno (returned by C
> > > rename
> > > function)
> > > +//=C2=A0=C2=A0 0: Success
> > > +//=C2=A0=C2=A0 1: file was not moved, but is already in <dest_path>
> > > +//=C2=A0=C2=A0 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) {
> > > +=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, filep=
attern);
> > > +
> > > +=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_=
path) + 1 + strlen(filename) +
> > > 1)
> > > * sizeof(char));
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dest =3D malloc((strlen(des=
t_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_p=
ath, filename);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sprintf(dest, "%s/%s", dest=
_path, filename);
> >=20
> > See my example above how to allocate a string (or actually not do
> > that at all by using a stack variable).
> >=20
>=20
> 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 ?


>=20
> > 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_d=
ir(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_IXGR=
P +
> > > 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_S=
IZE -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_pa=
ttern(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 snp=
rintf(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 snp=
rintf(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 snp=
rintf(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 <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) {
> > > +=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, fi=
lepattern);
> > > +=C2=A0=C2=A0=C2=A0 char *disabled =3D find_file_in_dir(disabled_path,
> > > filepattern);
> > > +
> > > +=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 enab=
led 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 disa=
bled 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 =
available 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(s=
tderr, "\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 argume=
nt is valid
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!is_valid_argument_alnum=
(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=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/pakfi=
re/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(s=
tderr, "\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], "=
stop") =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], "=
restart") =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], "=
reload") =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], "=
status") =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], "=
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=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], "=
disable") =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 =
arguments.\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 t=
o 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
>=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.
>=20
> > > +
> > > +=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 ad=
d-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, &service=
scnt);
> > > +=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 fpr=
intf(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 fpr=
intf(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

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
>=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 ++s=
ervices;
> > > +=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", err=
ormsg);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 }
> > > +=C2=A0=C2=A0=C2=A0=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 ++s=
ervices;
> > > +=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 ++s=
ervices;
> > > +=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=
 for 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 fpr=
intf(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 ++a=
ctioned;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ++s=
ervices;
> > > +=C2=A0=C2=A0=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 argu=
ment 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 && actio=
ned =3D=3D 0) {
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fprintf(stderr, "\nNo servi=
ce %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.
>=20
> 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.
>=20
> 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
>=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

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



--===============2264331725858375788==--