[BusyBox-cvs] busybox/applets applets.c,1.20,1.21
Manuel Novoa III
mjn3 at busybox.net
Sun Feb 1 10:03:07 UTC 2004
Update of /var/cvs/busybox/applets
In directory nail:/tmp/cvs-serv4181
Modified Files:
applets.c
Log Message:
Rewrite parse_config_file(). Among the old version's problems:
No checking for lines that were too long.
No checking that fgets returning NULL was actually due to EOF.
Various whitespace handling inconsistencies.
Bloat (switches and multiple identical function calls).
Failure to check for trailing characters in some cases.
Dynamicly allocated memory was not free()d on error.
Given that this controls suid/sgid behavior, the sloppy coding was
really inexcusable. :-(
Index: applets.c
===================================================================
RCS file: /var/cvs/busybox/applets/applets.c,v
retrieving revision 1.20
retrieving revision 1.21
diff -u -d -r1.20 -r1.21
--- applets.c 26 May 2003 14:09:12 -0000 1.20
+++ applets.c 1 Feb 2004 10:03:05 -0000 1.21
@@ -29,6 +29,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <assert.h>
#include "busybox.h"
#undef APPLET
@@ -53,9 +54,7 @@
#include "pwd_.h"
#include "grp_.h"
-static int parse_config_file (void);
-
-static int config_ok;
+static void parse_config_file (void);
#define CONFIG_FILE "/etc/busybox.conf"
@@ -127,7 +126,7 @@
#ifdef CONFIG_FEATURE_SUID_CONFIG
if (recurse_level == 0)
- config_ok = parse_config_file ();
+ parse_config_file ();
#endif
recurse_level++;
@@ -193,7 +192,7 @@
uid_t rgid = getgid ();
#ifdef CONFIG_FEATURE_SUID_CONFIG
- if (config_ok) {
+ if (suid_config) {
struct BB_suid_config *sct;
for (sct = suid_config; sct; sct = sct->m_next) {
@@ -253,191 +252,237 @@
#ifdef CONFIG_FEATURE_SUID_CONFIG
-
-#define parse_error(x) { err=x; goto pe_label; }
-
-
-int
-parse_config_file (void)
+/* This should probably be a libbb routine. In that case,
+ * I'd probably rename it to something like bb_trimmed_slice.
+ */
+static char *get_trimmed_slice(char *s, char *e)
{
- struct stat st;
- char *err = 0;
- FILE *f = 0;
- int lc = 0;
+ /* First, consider the value at e to be nul and back up until we
+ * reach a non-space char. Set the char after that (possibly at
+ * the original e) to nul. */
+ while (e-- > s) {
+ if (!isspace(*e)) {
+ break;
+ }
+ }
+ e[1] = 0;
- suid_config = 0;
+ /* Next, advance past all leading space and return a ptr to the
+ * first non-space char; possibly the terminating nul. */
+ return (char *) bb_skip_whitespace(s);
+}
- /* is there a config file ? */
- if (stat (CONFIG_FILE, &st) == 0) {
- /* is it owned by root with no write perm. for group and others ? */
- if (S_ISREG (st.st_mode) && (st.st_uid == 0)
- && (!(st.st_mode & (S_IWGRP | S_IWOTH)))) {
- /* that's ok .. then try to open it */
- f = fopen (CONFIG_FILE, "r");
- if (f) {
- char buffer[256];
- int section = 0;
-
- while (fgets (buffer, sizeof (buffer) - 1, f)) {
- char c = buffer[0];
- char *p;
+#define parse_error(x) { err=x; goto pe_label; }
- lc++;
+/* Don't depend on the tools to combine strings. */
+static const char config_file[] = CONFIG_FILE;
- p = strchr (buffer, '#');
- if (p)
- *p = 0;
- p = buffer + bb_strlen (buffer);
- while ((p > buffer) && isspace (*--p))
- *p = 0;
+/* There are 4 chars + 1 nul for each of user/group/other. */
+static const char mode_chars[] = "Ssx-\0Ssx-\0Ttx-";
- if (p == buffer)
- continue;
+/* We don't supply a value for the nul, so an index adjustment is
+ * necessary below. Also, we use unsigned short here to save some
+ * space even though these are really mode_t values. */
+static const unsigned short mode_mask[] = {
+ /* SST sst xxx --- */
+ S_ISUID, S_ISUID|S_IXUSR, S_IXUSR, 0, /* user */
+ S_ISGID, S_ISGID|S_IXGRP, S_IXGRP, 0, /* group */
+ 0, S_IXOTH, S_IXOTH, 0 /* other */
+};
- if (c == '[') {
- p = strchr (buffer, ']');
+static void parse_config_file(void)
+{
+ struct BB_suid_config *sct_head;
+ struct BB_suid_config *sct;
+ struct BB_applet *applet;
+ FILE *f;
+ char *err;
+ char *s;
+ char *e;
+ int i, lc, section;
+ char buffer[256];
+ struct stat st;
- if (!p || (p == (buffer + 1))) /* no matching ] or empty [] */
- parse_error ("malformed section header");
+ assert(!suid_config); /* Should be set to NULL by bss init. */
- *p = 0;
+ if ((stat(config_file, &st) != 0) /* No config file? */
+ || !S_ISREG(st.st_mode) /* Not a regular file? */
+ || (st.st_uid != 0) /* Not owned by root? */
+ || (st.st_mode & (S_IWGRP | S_IWOTH)) /* Writable by non-root? */
+ || !(f = fopen(config_file, "r")) /* Can not open? */
+ ) {
+ return;
+ }
- if (strcasecmp (buffer + 1, "SUID") == 0)
- section = 1;
- else
- section = -1; /* unknown section - just skip */
- } else if (section) {
- switch (section) {
- case 1:{ /* SUID */
- int l;
- struct BB_applet *applet;
+ sct_head = NULL;
+ section = lc = 0;
- p = strchr (buffer, '='); /* <key>[::space::]*=[::space::]*<value> */
+ do {
+ s = buffer;
- if (!p || (p == (buffer + 1))) /* no = or key is empty */
- parse_error ("malformed keyword");
+ if (!fgets(s, sizeof(buffer), f)) { /* Are we done? */
+ if (ferror(f)) { /* Make sure it wasn't a read error. */
+ parse_error("reading");
+ }
+ fclose(f);
+ suid_config = sct_head; /* Success, so set the pointer. */
+ return;
+ }
- l = p - buffer;
- while (isspace (buffer[--l])) {
- /* skip whitespace */
- }
+ lc++; /* Got a (partial) line. */
- buffer[l + 1] = 0;
+ /* If a line is too long for our buffer, we consider it an error.
+ * The following test does mistreat one corner case though.
+ * If the final line of the file does not end with a newline and
+ * yet exactly fills the buffer, it will be treated as too long
+ * even though there isn't really a problem. But it isn't really
+ * worth adding code to deal with such an unlikely situation, and
+ * we do err on the side of caution. Besides, the line would be
+ * too long if it did end with a newline. */
+ if (!strchr(s, '\n') && !feof(f)) {
+ parse_error("line too long");
+ }
- if ((applet = find_applet_by_name (buffer))) {
- struct BB_suid_config *sct =
- xmalloc (sizeof (struct BB_suid_config));
+ /* Trim leading and trailing whitespace, ignoring comments, and
+ * check if the resulting string is empty. */
+ if (!*(s = get_trimmed_slice(s, strchrnul(s, '#')))) {
+ continue;
+ }
- sct->m_applet = applet;
- sct->m_next = suid_config;
- suid_config = sct;
+ /* Check for a section header. */
- while (isspace (*++p)) {
- /* skip whitespace */
- }
+ if (*s == '[') {
+ /* Unlike the old code, we ignore leading and trailing
+ * whitespace for the section name. We also require that
+ * there are no stray characters after the closing bracket. */
+ if (!(e = strchr(s, ']')) /* Missing right bracket? */
+ || e[1] /* Trailing characters? */
+ || !*(s = get_trimmed_slice(s+1, e)) /* Missing name? */
+ ) {
+ parse_error("section header");
+ }
+ /* Right now we only have one section so just check it.
+ * If more sections are added in the future, please don't
+ * resort to cascading ifs with multiple strcasecmp calls.
+ * That kind of bloated code is all too common. A loop
+ * and a string table would be a better choice unless the
+ * number of sections is very small. */
+ if (strcasecmp(s, "SUID") == 0) {
+ section = 1;
+ continue;
+ }
+ section = -1; /* Unknown section so set to skip. */
+ continue;
+ }
- sct->m_mode = 0;
+ /* Process sections. */
- switch (*p++) {
- case 'S':
- sct->m_mode |= S_ISUID;
- break;
- case 's':
- sct->m_mode |= S_ISUID;
- /* no break */
- case 'x':
- sct->m_mode |= S_IXUSR;
- break;
- case '-':
- break;
- default:
- parse_error ("invalid user mode");
- }
+ if (section == 1) { /* SUID */
+ /* Since we trimmed leading and trailing space above, we're
+ * now looking for strings of the form
+ * <key>[::space::]*=[::space::]*<value>
+ * where both key and value could contain inner whitespace. */
- switch (*p++) {
- case 's':
- sct->m_mode |= S_ISGID;
- /* no break */
- case 'x':
- sct->m_mode |= S_IXGRP;
- break;
- case 'S':
- break;
- case '-':
- break;
- default:
- parse_error ("invalid group mode");
- }
+ /* First get the key (an applet name in our case). */
+ if (!!(e = strchr(s, '='))) {
+ s = get_trimmed_slice(s, e);
+ }
+ if (!e || !*s) { /* Missing '=' or empty key. */
+ parse_error("keyword");
+ }
- switch (*p) {
- case 't':
- case 'x':
- sct->m_mode |= S_IXOTH;
- break;
- case 'T':
- case '-':
- break;
- default:
- parse_error ("invalid other mode");
- }
+ /* Ok, we have an applet name. Process the rhs if this
+ * applet is currently built in and ignore it otherwise.
+ * Note: This can hide config file bugs which only pop
+ * up when the busybox configuration is changed. */
+ if ((applet = find_applet_by_name(s))) {
+ /* Note: We currently don't check for duplicates!
+ * The last config line for each applet will be the
+ * one used since we insert at the head of the list.
+ * I suppose this could be considered a feature. */
+ sct = xmalloc(sizeof(struct BB_suid_config));
+ sct->m_applet = applet;
+ sct->m_mode = 0;
+ sct->m_next = sct_head;
+ sct_head = sct;
- while (isspace (*++p)) {
- /* skip whitespace */
- }
+ /* Get the specified mode. */
- if (isdigit (*p)) {
- sct->m_uid = strtol (p, &p, 10);
- if (*p++ != '.')
- parse_error ("parsing <uid>.<gid>");
- } else {
- struct passwd *pwd;
- char *p2 = strchr (p, '.');
+ e = (char *) bb_skip_whitespace(e+1);
- if (!p2)
- parse_error ("parsing <uid>.<gid>");
+ for (i=0 ; i < 3 ; i++) {
+ const char *q;
+ if (!*(q = strchrnul(mode_chars + 5*i, *e++))) {
+ parse_error("mode");
+ }
+ /* Adjust by -i to account for nul. */
+ sct->m_mode |= mode_mask[(q - mode_chars) - i];
+ }
- *p2 = 0;
- pwd = getpwnam (p);
+ /* Now get the the user/group info. */
+
+ s = (char *) bb_skip_whitespace(e);
- if (!pwd)
- parse_error ("invalid user name");
+ /* Note: We require whitespace between the mode and the
+ * user/group info. */
+ if ((s == e) || !(e = strchr(s, '.'))) {
+ parse_error("<uid>.<gid>");
+ }
+ *e++ = 0;
- sct->m_uid = pwd->pw_uid;
- p = p2 + 1;
- }
- if (isdigit (*p))
- sct->m_gid = strtol (p, &p, 10);
- else {
- struct group *grp = getgrnam (p);
+ /* We can't use get_ug_id here since it would exit()
+ * if a uid or gid was not found. Oh well... */
+ {
+ char *e2;
- if (!grp)
- parse_error ("invalid group name");
+ sct->m_uid = strtoul(s, &e2, 10);
+ if (*e2 || (s == e2)) {
+ struct passwd *pwd;
+ if (!(pwd = getpwnam(s))) {
+ parse_error("user");
+ }
+ sct->m_uid = pwd->pw_uid;
+ }
- sct->m_gid = grp->gr_gid;
- }
+ sct->m_gid = strtoul(e, &e2, 10);
+ if (*e2 || (e == e2)) {
+ struct group *grp;
+ if (!(grp = getgrnam(e))) {
+ parse_error("group");
+ }
+ sct->m_gid = grp->gr_gid;
+ }
}
- break;
- }
- default: /* unknown - skip */
- break;
}
- } else
- parse_error ("keyword not within section");
+ continue;
}
- fclose (f);
- return 1;
- }
- }
- }
- return 0; /* no config file or not readable (not an error) */
-pe_label:
- fprintf (stderr, "Parse error in %s, line %d: %s\n", CONFIG_FILE, lc, err);
+ /* Unknown sections are ignored. */
- if (f)
- fclose (f);
- return 0;
+ /* Encountering configuration lines prior to seeing a
+ * section header is treated as an error. This is how
+ * the old code worked, but it may not be desireable.
+ * We may want to simply ignore such lines in case they
+ * are used in some future version of busybox. */
+ if (!section) {
+ parse_error("keyword outside section");
+ }
+
+ } while (1);
+
+ pe_label:
+ fprintf(stderr, "Parse error in %s, line %d: %s\n",
+ config_file, lc, err);
+
+ fclose(f);
+ /* Release any allocated memory before returning. */
+ while (sct_head) {
+ sct = sct_head->m_next;
+ free(sct_head);
+ sct_head = sct;
+ }
+ return;
}
#endif
@@ -446,9 +491,9 @@
/* END CODE */
/*
-Local Variables:
-c-file-style: "linux"
-c-basic-offset: 4
-tab-width: 4
+ Local Variables:
+ c-file-style: "linux"
+ c-basic-offset: 4
+ tab-width: 4
End:
*/
More information about the busybox-cvs
mailing list