Re: login_fbtab glob
On Wed, 20 Apr 2022 09:18:20 -0500, joshua stein wrote: > I like it. Here's the full diff: OK millert@ - todd
Re: login_fbtab glob
On Fri, 15 Apr 2022 at 18:54:49 -0600, Todd C. Miller wrote:
> On Fri, 15 Apr 2022 13:28:33 -0500, joshua stein wrote:
>
> > Anyone want to bikeshed this language?
>
> I think it is more helpful to refer to glob(7) than glob(3).
> Perhaps something like this.
I like it. Here's the full diff:
Index: lib/libutil/login_fbtab.c
===
RCS file: /cvs/src/lib/libutil/login_fbtab.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 login_fbtab.c
--- lib/libutil/login_fbtab.c 27 Nov 2015 01:57:59 - 1.16
+++ lib/libutil/login_fbtab.c 20 Apr 2022 14:17:08 -
@@ -61,8 +61,8 @@
#include
#include
-#include
#include
+#include
#include
#include
#include
@@ -134,49 +134,31 @@ login_fbtab(const char *tty, uid_t uid,
static void
login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid)
{
- charbuf[PATH_MAX];
- size_t pathlen = strlen(path);
- DIR *dir;
- struct dirent *ent;
+ glob_t g;
+ size_t n;
+ char*gpath;
- if (pathlen >= sizeof(buf)) {
+ if (strlen(path) >= PATH_MAX) {
errno = ENAMETOOLONG;
syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path);
return;
}
- if (strcmp("/*", path + pathlen - 2) != 0) {
- if (chmod(path, mask) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path);
- if (chown(path, uid, gid) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path);
- } else {
- /*
-* This is a wildcard directory (/path/to/whatever/ * ).
-* Make a copy of path without the trailing '*' (but leave
-* the trailing '/' so we can append directory entries.)
-*/
- memcpy(buf, path, pathlen - 1);
- buf[pathlen - 1] = '\0';
- if ((dir = opendir(buf)) == NULL) {
- syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB,
- path);
- return;
- }
+ if (glob(path, GLOB_NOSORT, NULL, ) != 0) {
+ if (errno != ENOENT)
+ syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path);
+ globfree();
+ return;
+ }
- while ((ent = readdir(dir)) != NULL) {
- if (strcmp(ent->d_name, ".") != 0 &&
- strcmp(ent->d_name, "..") != 0) {
- buf[pathlen - 1] = '\0';
- if (strlcat(buf, ent->d_name, sizeof(buf))
- >= sizeof(buf)) {
- errno = ENAMETOOLONG;
- syslog(LOG_ERR, "%s: %s: %m",
- _PATH_FBTAB, path);
- } else
- login_protect(buf, mask, uid, gid);
- }
- }
- closedir(dir);
+ for (n = 0; n < g.gl_matchc; n++) {
+ gpath = g.gl_pathv[n];
+
+ if (chmod(gpath, mask) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB,
gpath);
+ if (chown(gpath, uid, gid) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB,
gpath);
}
+
+ globfree();
}
Index: share/man/man5/fbtab.5
===
RCS file: /cvs/src/share/man/man5/fbtab.5,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 fbtab.5
--- share/man/man5/fbtab.5 8 Sep 2014 01:27:55 - 1.14
+++ share/man/man5/fbtab.5 20 Apr 2022 14:17:08 -
@@ -51,15 +51,11 @@ An octal permission number (0600), as us
.It Other devices
The final field is a colon
.Pq Ql \&:
-delimited list of devices (e.g.,
-.Dq /dev/console:/dev/fd0a ) .
-All device names are absolute paths.
-A path that ends in
-.Dq /\&*
-refers to all directory entries except
-.Dq \&.
-and
-.Dq \&.\&. .
+delimited list of device paths (e.g.,
+.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) .
+Device paths may include shell-style globbing patterns (see
+.Xr glob 7 ) ,
+potentially matching multiple devices.
.El
.Pp
The
@@ -84,5 +80,6 @@ the files once again belonging to root.
.Xr login 1 ,
.Xr login_fbtab 3 ,
.Xr init 8
+.Xr glob 7
.Sh AUTHORS
.An Guido van Rooij
Re: login_fbtab glob
On Fri, 15 Apr 2022 13:28:33 -0500, joshua stein wrote: > Anyone want to bikeshed this language? I think it is more helpful to refer to glob(7) than glob(3). Perhaps something like this. - todd Index: share/man/man5/fbtab.5 === RCS file: /cvs/src/share/man/man5/fbtab.5,v retrieving revision 1.14 diff -u -p -u -r1.14 fbtab.5 --- share/man/man5/fbtab.5 8 Sep 2014 01:27:55 - 1.14 +++ share/man/man5/fbtab.5 16 Apr 2022 00:52:58 - @@ -51,15 +51,11 @@ An octal permission number (0600), as us .It Other devices The final field is a colon .Pq Ql \&: -delimited list of devices (e.g., -.Dq /dev/console:/dev/fd0a ) . -All device names are absolute paths. -A path that ends in -.Dq /\&* -refers to all directory entries except -.Dq \&. -and -.Dq \&.\&. . +delimited list of device paths (e.g., +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . +Device paths may include shell-style globbing patterns (see +.Xr glob 7 ) , +potentially matching multiple devices. .El .Pp The @@ -83,6 +79,7 @@ the files once again belonging to root. .Sh SEE ALSO .Xr login 1 , .Xr login_fbtab 3 , +.Xr glob 7 , .Xr init 8 .Sh AUTHORS .An Guido van Rooij
Re: login_fbtab glob
Do we need to worry about symbolic links? I guess not, since they would only be owned by root? joshua stein wrote: > On Fri, 15 Apr 2022 at 12:17:12 -0600, Todd C. Miller wrote: > > On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > > > > > Thanks, both applied. > > > > Looks good to me but needs a man page update. > > Anyone want to bikeshed this language? > > > diff --git share/man/man5/fbtab.5 share/man/man5/fbtab.5 > index 13dfb634b67..18eade1fbfa 100644 > --- share/man/man5/fbtab.5 > +++ share/man/man5/fbtab.5 > @@ -51,15 +51,11 @@ An octal permission number (0600), as used by > .It Other devices > The final field is a colon > .Pq Ql \&: > -delimited list of devices (e.g., > -.Dq /dev/console:/dev/fd0a ) . > -All device names are absolute paths. > -A path that ends in > -.Dq /\&* > -refers to all directory entries except > -.Dq \&. > -and > -.Dq \&.\&. . > +delimited list of device paths (e.g., > +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . > +All device paths are processed through > +.Xr glob 3 > +to expand any wildcard characters and potentially match multiple devices. > .El > .Pp > The > @@ -84,5 +80,6 @@ the files once again belonging to root. > .Xr login 1 , > .Xr login_fbtab 3 , > .Xr init 8 > +.Xr glob 3 > .Sh AUTHORS > .An Guido van Rooij >
Re: login_fbtab glob
On Fri, 15 Apr 2022 at 12:17:12 -0600, Todd C. Miller wrote: > On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > > > Thanks, both applied. > > Looks good to me but needs a man page update. Anyone want to bikeshed this language? diff --git share/man/man5/fbtab.5 share/man/man5/fbtab.5 index 13dfb634b67..18eade1fbfa 100644 --- share/man/man5/fbtab.5 +++ share/man/man5/fbtab.5 @@ -51,15 +51,11 @@ An octal permission number (0600), as used by .It Other devices The final field is a colon .Pq Ql \&: -delimited list of devices (e.g., -.Dq /dev/console:/dev/fd0a ) . -All device names are absolute paths. -A path that ends in -.Dq /\&* -refers to all directory entries except -.Dq \&. -and -.Dq \&.\&. . +delimited list of device paths (e.g., +.Dq /dev/console:/dev/fd0a:/dev/wskbd* ) . +All device paths are processed through +.Xr glob 3 +to expand any wildcard characters and potentially match multiple devices. .El .Pp The @@ -84,5 +80,6 @@ the files once again belonging to root. .Xr login 1 , .Xr login_fbtab 3 , .Xr init 8 +.Xr glob 3 .Sh AUTHORS .An Guido van Rooij
Re: login_fbtab glob
On Fri, 15 Apr 2022 13:12:03 -0500, joshua stein wrote: > Thanks, both applied. Looks good to me but needs a man page update. - todd
Re: login_fbtab glob
On Fri, 15 Apr 2022 at 09:32:46 -0600, Todd C. Miller wrote:
> On Thu, 14 Apr 2022 17:52:37 -0500, joshua stein wrote:
>
> > login_fbtab(3) supports wildcards but only for every file in a
> > directory (/path/*).
> >
> > This makes it use glob(3) so it can also support more specific
> > wildcards like /path/file*
>
> Yes, it is better to use glob(3) than something custom.
> Comments inline.
Thanks, both applied.
diff --git lib/libutil/login_fbtab.c lib/libutil/login_fbtab.c
index 5eacf4f65ff..b81bc8e38e4 100644
--- lib/libutil/login_fbtab.c
+++ lib/libutil/login_fbtab.c
@@ -61,8 +61,8 @@
#include
#include
-#include
#include
+#include
#include
#include
#include
@@ -134,49 +134,31 @@ login_fbtab(const char *tty, uid_t uid, gid_t gid)
static void
login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid)
{
- charbuf[PATH_MAX];
- size_t pathlen = strlen(path);
- DIR *dir;
- struct dirent *ent;
+ glob_t g;
+ size_t n;
+ char*gpath;
- if (pathlen >= sizeof(buf)) {
+ if (strlen(path) >= PATH_MAX) {
errno = ENAMETOOLONG;
syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path);
return;
}
- if (strcmp("/*", path + pathlen - 2) != 0) {
- if (chmod(path, mask) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path);
- if (chown(path, uid, gid) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path);
- } else {
- /*
-* This is a wildcard directory (/path/to/whatever/ * ).
-* Make a copy of path without the trailing '*' (but leave
-* the trailing '/' so we can append directory entries.)
-*/
- memcpy(buf, path, pathlen - 1);
- buf[pathlen - 1] = '\0';
- if ((dir = opendir(buf)) == NULL) {
- syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB,
- path);
- return;
- }
+ if (glob(path, GLOB_NOSORT, NULL, ) != 0) {
+ if (errno != ENOENT)
+ syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path);
+ globfree();
+ return;
+ }
- while ((ent = readdir(dir)) != NULL) {
- if (strcmp(ent->d_name, ".") != 0 &&
- strcmp(ent->d_name, "..") != 0) {
- buf[pathlen - 1] = '\0';
- if (strlcat(buf, ent->d_name, sizeof(buf))
- >= sizeof(buf)) {
- errno = ENAMETOOLONG;
- syslog(LOG_ERR, "%s: %s: %m",
- _PATH_FBTAB, path);
- } else
- login_protect(buf, mask, uid, gid);
- }
- }
- closedir(dir);
+ for (n = 0; n < g.gl_matchc; n++) {
+ gpath = g.gl_pathv[n];
+
+ if (chmod(gpath, mask) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB,
gpath);
+ if (chown(gpath, uid, gid) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB,
gpath);
}
+
+ globfree();
}
Re: login_fbtab glob
On Thu, 14 Apr 2022 17:52:37 -0500, joshua stein wrote:
> login_fbtab(3) supports wildcards but only for every file in a
> directory (/path/*).
>
> This makes it use glob(3) so it can also support more specific
> wildcards like /path/file*
Yes, it is better to use glob(3) than something custom.
Comments inline.
- todd
> diff --git lib/libutil/login_fbtab.c lib/libutil/login_fbtab.c
> index 5eacf4f65ff..39621a0cde4 100644
> --- lib/libutil/login_fbtab.c
> +++ lib/libutil/login_fbtab.c
> @@ -61,8 +61,8 @@
> #include
>
> #include
> -#include
> #include
> +#include
> #include
> #include
> #include
> @@ -134,49 +134,32 @@ login_fbtab(const char *tty, uid_t uid, gid_t gid)
> static void
> login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid)
> {
> - charbuf[PATH_MAX];
> - size_t pathlen = strlen(path);
> - DIR *dir;
> - struct dirent *ent;
> + glob_t g;
> + size_t n;
> + char*gpath;
>
> - if (pathlen >= sizeof(buf)) {
> + if (strlen(path) > PATH_MAX) {
The test should be >= PATH_MAX since PATH_MAX includes space for
the NUL.
> errno = ENAMETOOLONG;
> syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path);
> return;
> }
>
> - if (strcmp("/*", path + pathlen - 2) != 0) {
> - if (chmod(path, mask) && errno != ENOENT)
> - syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path)
> ;
> - if (chown(path, uid, gid) && errno != ENOENT)
> - syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path)
> ;
> - } else {
> - /*
> - * This is a wildcard directory (/path/to/whatever/ * ).
> - * Make a copy of path without the trailing '*' (but leave
> - * the trailing '/' so we can append directory entries.)
> - */
> - memcpy(buf, path, pathlen - 1);
> - buf[pathlen - 1] = '\0';
> - if ((dir = opendir(buf)) == NULL) {
> - syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB,
> - path);
> - return;
> - }
> + g.gl_offs = 0;
> + if (glob(path, GLOB_DOOFFS, NULL, ) != 0) {
I don't think you need GLOB_DOOFFS and g.gl_offs here since you are
not reserving slots in gl_pathv. I would just use GLOB_NOSORT
unless you need things to be sorted.
> + if (errno != ENOENT)
> + syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path);
> + globfree();
> + return;
> + }
>
> - while ((ent = readdir(dir)) != NULL) {
> - if (strcmp(ent->d_name, ".") != 0 &&
> - strcmp(ent->d_name, "..") != 0) {
> - buf[pathlen - 1] = '\0';
> - if (strlcat(buf, ent->d_name, sizeof(buf))
> - >= sizeof(buf)) {
> - errno = ENAMETOOLONG;
> - syslog(LOG_ERR, "%s: %s: %m",
> - _PATH_FBTAB, path);
> - } else
> - login_protect(buf, mask, uid, gid);
> - }
> - }
> - closedir(dir);
> + for (n = 0; n < g.gl_matchc; n++) {
> + gpath = g.gl_pathv[n];
> +
> + if (chmod(gpath, mask) && errno != ENOENT)
> + syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, gpath
> );
> + if (chown(gpath, uid, gid) && errno != ENOENT)
> + syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, gpath
> );
> }
> +
> + globfree();
> }
login_fbtab glob
login_fbtab(3) supports wildcards but only for every file in a
directory (/path/*).
This makes it use glob(3) so it can also support more specific
wildcards like /path/file*
diff --git lib/libutil/login_fbtab.c lib/libutil/login_fbtab.c
index 5eacf4f65ff..39621a0cde4 100644
--- lib/libutil/login_fbtab.c
+++ lib/libutil/login_fbtab.c
@@ -61,8 +61,8 @@
#include
#include
-#include
#include
+#include
#include
#include
#include
@@ -134,49 +134,32 @@ login_fbtab(const char *tty, uid_t uid, gid_t gid)
static void
login_protect(const char *path, mode_t mask, uid_t uid, gid_t gid)
{
- charbuf[PATH_MAX];
- size_t pathlen = strlen(path);
- DIR *dir;
- struct dirent *ent;
+ glob_t g;
+ size_t n;
+ char*gpath;
- if (pathlen >= sizeof(buf)) {
+ if (strlen(path) > PATH_MAX) {
errno = ENAMETOOLONG;
syslog(LOG_ERR, "%s: %s: %m", _PATH_FBTAB, path);
return;
}
- if (strcmp("/*", path + pathlen - 2) != 0) {
- if (chmod(path, mask) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB, path);
- if (chown(path, uid, gid) && errno != ENOENT)
- syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB, path);
- } else {
- /*
-* This is a wildcard directory (/path/to/whatever/ * ).
-* Make a copy of path without the trailing '*' (but leave
-* the trailing '/' so we can append directory entries.)
-*/
- memcpy(buf, path, pathlen - 1);
- buf[pathlen - 1] = '\0';
- if ((dir = opendir(buf)) == NULL) {
- syslog(LOG_ERR, "%s: opendir(%s): %m", _PATH_FBTAB,
- path);
- return;
- }
+ g.gl_offs = 0;
+ if (glob(path, GLOB_DOOFFS, NULL, ) != 0) {
+ if (errno != ENOENT)
+ syslog(LOG_ERR, "%s: glob(%s): %m", _PATH_FBTAB, path);
+ globfree();
+ return;
+ }
- while ((ent = readdir(dir)) != NULL) {
- if (strcmp(ent->d_name, ".") != 0 &&
- strcmp(ent->d_name, "..") != 0) {
- buf[pathlen - 1] = '\0';
- if (strlcat(buf, ent->d_name, sizeof(buf))
- >= sizeof(buf)) {
- errno = ENAMETOOLONG;
- syslog(LOG_ERR, "%s: %s: %m",
- _PATH_FBTAB, path);
- } else
- login_protect(buf, mask, uid, gid);
- }
- }
- closedir(dir);
+ for (n = 0; n < g.gl_matchc; n++) {
+ gpath = g.gl_pathv[n];
+
+ if (chmod(gpath, mask) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chmod(%s): %m", _PATH_FBTAB,
gpath);
+ if (chown(gpath, uid, gid) && errno != ENOENT)
+ syslog(LOG_ERR, "%s: chown(%s): %m", _PATH_FBTAB,
gpath);
}
+
+ globfree();
}
