SQLite Forum

should use eaccess instead of access to check temporary file directory

should use eaccess instead of access to check temporary file directory

(1) By jjm2473 on 2020-08-16 03:38:25

The 'access' syscall uses REAL uid to check aceess,
we should use EFFECTIVE uid with 'eaccess' call.
See man page for their difference.

## Detail:
Issue in forked-daapd in OpenWRT,
before patched, forked-daapd scanning multi files or doing database vacuum, 
 'unable to open database file' may appears in log.
Using strace to trace it, shows that:
setresgid(-1, 3003, -1)                 = 0
setresuid(-1, 190, -1)                  = 0
newfstatat(AT_FDCWD, "/var/tmp", {st_mode=S_IFDIR|0755, st_size=40, ...}, 0) = 0
faccessat(AT_FDCWD, "/var/tmp", W_OK|X_OK) = 0
faccessat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", F_OK) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE|O_NOFOLLOW|O_CLOEXEC, 0600) = -1 EACCES (Permission denied)
openat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", O_RDONLY|O_EXCL|O_LARGEFILE|O_NOFOLLOW|O_CLOEXEC) = -1 ENOENT (No such file or directory)
As shown above, forked-daapd was run by root, then it sets its effective uid to 190, 
then sqlite trying to check '/var/tmp' writeable with `faccessat(AT_FDCWD, "/var/tmp", W_OK|X_OK)`, kernel says ok, but failed when actually create file with `openat`.
Because `faccessat` using real uid to check access by default, but `openat` using effective uid.
PS, The information of '/var/tmp' :
drwxr-xr-x    2 root     root            40 Aug 16 10:29 tmp

## Patch:

--- a/sqlite3.c
+++ b/sqlite3.c
@@ -33842,7 +33842,7 @@ static struct unix_syscall {
   { "close",        (sqlite3_syscall_ptr)close,      0  },
 #define osClose     ((int(*)(int))aSyscall[1].pCurrent)
-  { "access",       (sqlite3_syscall_ptr)access,     0  },
+  { "access",       (sqlite3_syscall_ptr)eaccess,     0  },
 #define osAccess    ((int(*)(const char*,int))aSyscall[2].pCurrent)
   { "getcwd",       (sqlite3_syscall_ptr)getcwd,     0  },

(2) By Stephan Beal (stephan) on 2020-08-16 03:43:49 in reply to 1 [link]

`man eaccess` says it's not a standard API...

       euidaccess, eaccess - check effective user's permissions for a file

       #define _GNU_SOURCE             /* See feature_test_macros(7) */
       #include <unistd.h>
       These functions are nonstandard.  Some other systems have an eaccess() function.

(3) By jjm2473 on 2020-08-16 10:13:40 in reply to 2 [link]

No, eaccess is NOT portable.
Maybe we should find solution in libc.

In musl libc, eaccess is define as:


#define _GNU_SOURCE
#include <unistd.h>
#include <fcntl.h>
#include "libc.h"

int euidaccess(const char *filename, int amode)
	return faccessat(AT_FDCWD, filename, amode, AT_EACCESS);

weak_alias(euidaccess, eaccess);




#include <unistd.h>
#include <fcntl.h>
#include <sys/wait.h>
#include "syscall.h"
#include "pthread_impl.h"

struct ctx {
	int fd;
	const char *filename;
	int amode;
	int p;

static int checker(void *p)
	struct ctx *c = p;
	int ret;
	if (__syscall(SYS_setregid, __syscall(SYS_getegid), -1)
	    || __syscall(SYS_setreuid, __syscall(SYS_geteuid), -1))
		__syscall(SYS_exit, 1);
	ret = __syscall(SYS_faccessat, c->fd, c->filename, c->amode, 0);
	__syscall(SYS_write, c->p, &ret, sizeof ret);
	return 0;

int faccessat(int fd, const char *filename, int amode, int flag)
	if (!flag || (flag==AT_EACCESS && getuid()==geteuid() && getgid()==getegid()))
		return syscall(SYS_faccessat, fd, filename, amode, flag);

	if (flag != AT_EACCESS)
		return __syscall_ret(-EINVAL);

	char stack[1024];
	sigset_t set;
	pid_t pid;
	int status;
	int ret, p[2];

	if (pipe2(p, O_CLOEXEC)) return __syscall_ret(-EBUSY);
	struct ctx c = { .fd = fd, .filename = filename, .amode = amode, .p = p[1] };

	pid = __clone(checker, stack+sizeof stack, 0, &c);
	__syscall(SYS_close, p[1]);

	if (pid<0 || __syscall(SYS_read, p[0], &ret, sizeof ret) != sizeof(ret))
		ret = -EBUSY;
	__syscall(SYS_close, p[0]);
	__syscall(SYS_wait4, pid, &status, __WCLONE, 0);


	return __syscall_ret(ret);


So, it calls clone to run checker.

But is `clone` a good way? (Some system may strict clone syscall)

Do you have other ways? 

Can we just change `unixTempFileDir` to really create a temporary file to check the directory is writable?