9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
@ 2023-01-03 22:29 Xiao-Yong Jin
  2023-01-06 14:21 ` cinap_lenrek
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Xiao-Yong Jin @ 2023-01-03 22:29 UTC (permalink / raw)
  To: 9front

Under posix drawterm, creating new file or dir under /mnt/term
always ends up having perm bits 0666 or 0777.  The patch below
includes the parent dir's perm in fscreate.  It also fix the
uid/gid the same as the parent dir.

diff --git a/kern/devfs-posix.c b/kern/devfs-posix.c
index 31eca3c..c715072 100644
--- a/kern/devfs-posix.c
+++ b/kern/devfs-posix.c
@@ -244,8 +244,9 @@ fscreate(Chan *c, char *name, int mode, ulong perm)
                  fd = open(path, 0);
                if(fd >= 0) {
+                       perm &= uif->mode & 0777;
                        chmod(path, perm & 0777);
-                       chown(path, uif->uid, uif->uid);
+                       chown(path, uif->uid, uif->gid);
                }
                close(fd);
  @@ -260,7 +261,8 @@ fscreate(Chan *c, char *name, int mode, ulong perm)
                                close(fd);
                                fd = open(path, m);
                        }
-                       chmod(path, perm & 0777);
+                       perm &= uif->mode & 0666;
+                       chmod(path, perm & 0666);
                        chown(path, uif->uid, uif->gid);
                }
                if(fd < 0)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
  2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
@ 2023-01-06 14:21 ` cinap_lenrek
  2023-01-06 14:44 ` cinap_lenrek
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 14:21 UTC (permalink / raw)
  To: 9front

it looks ok, but it is still wrong no?

          The create request asks the file server to create a new file
          with the name supplied, in the directory (dir) represented
          by fid, and requires write permission in the directory.  The
          owner of the file is the implied user id of the request, the
          group of the file is the same as dir, and the permissions
          are the value of
                    perm & (~0666 | (dir.perm & 0666))
          if a regular file is being created and
                    perm & (~0777 | (dir.perm & 0777))
          if a directory is being created.  This means, for example,
          that if the create allows read permission to others, but the
          containing directory does not, then the created file will
          not allow others to read the file.

we'r changing the uid to the directory owner, but it should the the uid
of the drawterm process.

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
  2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
  2023-01-06 14:21 ` cinap_lenrek
@ 2023-01-06 14:44 ` cinap_lenrek
  2023-01-06 15:30   ` Xiao-Yong Jin
  2023-01-06 15:40 ` cinap_lenrek
  2023-01-06 18:20 ` cinap_lenrek
  3 siblings, 1 reply; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 14:44 UTC (permalink / raw)
  To: 9front

how about this?

--- a/./kern/devfs-posix.c
+++ b/./kern/devfs-posix.c
@@ -244,8 +244,8 @@
 
 		fd = open(path, 0);
 		if(fd >= 0) {
-			chmod(path, perm & 0777);
-			chown(path, uif->uid, uif->uid);
+			chmod(path, uif->mode & perm & 0777);
+			chown(path, -1, uif->gid);
 		}
 		close(fd);
 
@@ -260,8 +260,8 @@
 				close(fd);
 				fd = open(path, m);
 			}
-			chmod(path, perm & 0777);
-			chown(path, uif->uid, uif->gid);
+			chmod(path, uif->mode & perm & 0666);
+			chown(path, -1, uif->gid);
 		}
 		if(fd < 0)
 			error(strerror(errno));

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
  2023-01-06 14:44 ` cinap_lenrek
@ 2023-01-06 15:30   ` Xiao-Yong Jin
  0 siblings, 0 replies; 6+ messages in thread
From: Xiao-Yong Jin @ 2023-01-06 15:30 UTC (permalink / raw)
  To: 9front

better.

> On Jan 6, 2023, at 8:44 AM, cinap_lenrek@felloff.net wrote:
> 
> how about this?
> 
> --- a/./kern/devfs-posix.c
> +++ b/./kern/devfs-posix.c
> @@ -244,8 +244,8 @@
> 
> fd = open(path, 0);
> if(fd >= 0) {
> - chmod(path, perm & 0777);
> - chown(path, uif->uid, uif->uid);
> + chmod(path, uif->mode & perm & 0777);
> + chown(path, -1, uif->gid);
> }
> close(fd);
> 
> @@ -260,8 +260,8 @@
> close(fd);
> fd = open(path, m);
> }
> - chmod(path, perm & 0777);
> - chown(path, uif->uid, uif->gid);
> + chmod(path, uif->mode & perm & 0666);
> + chown(path, -1, uif->gid);
> }
> if(fd < 0)
> error(strerror(errno));
> 
> --
> cinap


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
  2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
  2023-01-06 14:21 ` cinap_lenrek
  2023-01-06 14:44 ` cinap_lenrek
@ 2023-01-06 15:40 ` cinap_lenrek
  2023-01-06 18:20 ` cinap_lenrek
  3 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 15:40 UTC (permalink / raw)
  To: 9front

hm... the next question is, why do we even do the chmod(),
when we can just pass the desired permissions (masked by
the parent directory) to open?

who the hell wrote this crazy code?

--
cinap

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm
  2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
                   ` (2 preceding siblings ...)
  2023-01-06 15:40 ` cinap_lenrek
@ 2023-01-06 18:20 ` cinap_lenrek
  3 siblings, 0 replies; 6+ messages in thread
From: cinap_lenrek @ 2023-01-06 18:20 UTC (permalink / raw)
  To: 9front

ok, i found some other issues with the code and
committed the following:

94f8d9a60fabb07dd7e37c806ca39eadbd883339 devfs: create handle OEXCL, mask permissions with parent directory, fix leaks
Hash: 94f8d9a60fabb07dd7e37c806ca39eadbd883339
Date: Fri Jan  6 19:17:58 CET 2023
Author: cinap_lenrek <cinap_lenrek@felloff.net>
Message: devfs: create handle OEXCL, mask permissions with parent directory, fix leaks

OEXCL has not been handled correctly. We have to
return an error if the file already exists.

The file-permissions should only be applied when
we create a new file.

When creating a new file, we mask te permissions
with the permissions of the parent directory
(see open(5)).

There where a few potential leaks. After creating
file file, we do a stat to get the qid information,
but this can potentially fail, eigther leaving the
Chan in the wrong state or leaking memory. Fix it.

diff -u .git/fs/object/eeeb7632dd77d90d1439bb31ca198c5061950301/tree/kern/devfs-posix.c .git/fs/object/94f8d9a60fabb07dd7e37c806ca39eadbd883339/tree/kern/devfs-posix.c
--- .git/fs/object/eeeb7632dd77d90d1439bb31ca198c5061950301/tree/kern/devfs-posix.c
+++ .git/fs/object/94f8d9a60fabb07dd7e37c806ca39eadbd883339/tree/kern/devfs-posix.c
@@ -170,51 +170,29 @@
 static Chan*
 fsopen(Chan *c, int mode)
 {
-	int m, isdir;
 	Ufsinfo *uif;
+	int omode;
 
-/*print("fsopen %s\n", chanpath(c));*/
-	m = mode & (OTRUNC|3);
-	switch(m) {
-	case 0:
-		break;
-	case 1:
-	case 1|16:
-		break;
-	case 2:	
-	case 0|16:
-	case 2|16:
-		break;
-	case 3:
-		break;
-	default:
-		error(Ebadarg);
-	}
+	omode = openmode(mode);
 
-	isdir = c->qid.type & QTDIR;
-
-	if(isdir && mode != OREAD)
-		error(Eperm);
-
-	m = fsomode(m & 3);
-	c->mode = openmode(mode);
-
 	uif = c->aux;
-	if(isdir) {
-		uif->dir = opendir(uif->path);
-		if(uif->dir == 0)
+	if(c->qid.type & QTDIR) {
+		if(omode != OREAD)
+			error(Eperm);
+		if((uif->dir = opendir(uif->path)) == NULL)
 			error(strerror(errno));
 	}	
 	else {
+		int m = fsomode(omode & 3);
 		if(mode & OTRUNC)
 			m |= O_TRUNC;
-		uif->fd = open(uif->path, m, 0666);
-		if(uif->fd < 0)
+		if((uif->fd = open(uif->path, m, 0666)) < 0)
 			error(strerror(errno));
 	}
 	uif->offset = 0;
 
 	c->offset = 0;
+	c->mode = omode;
 	c->flag |= COPEN;
 	return c;
 }
@@ -222,12 +200,13 @@
 static Chan*
 fscreate(Chan *c, char *name, int mode, ulong perm)
 {
-	int fd, m;
-	char *path;
 	struct stat stbuf;
 	Ufsinfo *uif;
+	char *path;
+	DIR *dir;
+	int fd, omode;
 
-	m = fsomode(mode&3);
+	omode = openmode(mode & ~OEXCL);
 
 	uif = c->aux;
 	path = catpath(uif->path, name);
@@ -235,49 +214,48 @@
 		free(path);
 		nexterror();
 	}
+	fd = -1;
+	dir = NULL;
 	if(perm & DMDIR) {
-		if(m)
+		if(omode != OREAD)
 			error(Eperm);
-
-		if(mkdir(path, perm & 0777) < 0)
-			error(strerror(errno));
-
-		fd = open(path, 0);
-		if(fd >= 0) {
-			chmod(path, perm & 0777);
-			chown(path, uif->uid, uif->uid);
+		if(mkdir(path, uif->mode & perm & 0777) < 0){
+			if(errno != EEXIST || mode & OEXCL)
+				error(strerror(errno));
 		}
-		close(fd);
-
-		uif->dir = opendir(path);
-		if(uif->dir == 0)
+		if((dir = opendir(path)) == NULL)
 			error(strerror(errno));
+		if(waserror()){
+			closedir(dir);
+			nexterror();
+		}
 	}
 	else {
-		fd = open(path, O_WRONLY|O_CREAT|O_TRUNC, 0666);
-		if(fd >= 0) {
-			if(m != 1) {
-				close(fd);
-				fd = open(path, m);
-			}
-			chmod(path, perm & 0777);
-			chown(path, uif->uid, uif->gid);
-		}
-		if(fd < 0)
+		int m = fsomode(omode & 3) | O_CREAT | O_TRUNC;
+		if(mode & OEXCL)
+			m |= O_EXCL;
+		if((fd = open(path, m, uif->mode & perm & 0666)) < 0)
 			error(strerror(errno));
-		uif->fd = fd;
+		if(waserror()){
+			close(fd);
+			nexterror();
+		}
 	}
 	if(stat(path, &stbuf) < 0)
 		error(strerror(errno));
+	c->qid = fsqid(&stbuf);
+	uif->fd = fd;
+	uif->dir = dir;
+	poperror();
 
 	free(uif->path);
 	uif->path = path;
+	uif->offset = 0;
 	poperror();
 
-	c->qid = fsqid(&stbuf);
 	c->offset = 0;
+	c->mode = omode;
 	c->flag |= COPEN;
-	c->mode = openmode(mode);
 	return c;
 }
 
@@ -604,13 +582,13 @@
 fsomode(int m)
 {
 	switch(m) {
-	case 0:			/* OREAD */
-	case 3:			/* OEXEC */
-		return 0;
-	case 1:			/* OWRITE */
-		return 1;
-	case 2:			/* ORDWR */
-		return 2;
+	case OREAD:
+	case OEXEC:
+		return O_RDONLY;
+	case OWRITE:
+		return O_WRONLY;
+	case ORDWR:
+		return O_RDWR;
 	}
 	error(Ebadarg);
 	return 0;

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-01-06 18:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-03 22:29 [9front] drawterm: kern/devfs-posix: fscreate does not respect parent dir's perm Xiao-Yong Jin
2023-01-06 14:21 ` cinap_lenrek
2023-01-06 14:44 ` cinap_lenrek
2023-01-06 15:30   ` Xiao-Yong Jin
2023-01-06 15:40 ` cinap_lenrek
2023-01-06 18:20 ` cinap_lenrek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).