[alsa-devel] [PATCH] alsactl: Store lockfile in /tmp
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Signed-off-by: Julian Scheel julian@jusst.de --- alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12]; + char *filename; char *nfile;
if (!do_lock) return 0; - nfile = malloc(strlen(file) + 6); + + /* only use the actual filename, not the path */ + filename = strrchr(file, '/'); + if (!filename) + filename = file; + + nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; } - strcpy(nfile, file); - strcat(nfile, ".lock"); + + sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
Jaroslav
Signed-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
Date 6.5.2014 16:53, Jaroslav Kysela wrote:
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR
I'm sorry, the filename variable is not mallocated, forget this part, please.
environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
JaroslavSigned-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
On 06.05.2014 16:53, Jaroslav Kysela wrote:
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
The patch does not allocate anything that was not allocated before. nfile was allocated before and is freed a few lines after the patch content. filename is just a pointer, not a newly allocated buffer. Using /var/lock instead of /tmp sounds sane, yes.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
We're running alsactl restore on startup of an embedded system which uses a read-only rootfs. So it can't create the lockfile in the default place and hence will not restore anything.
-Julian
JaroslavSigned-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
At Tue, 06 May 2014 17:00:47 +0200, Julian Scheel wrote:
On 06.05.2014 16:53, Jaroslav Kysela wrote:
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
The patch does not allocate anything that was not allocated before. nfile was allocated before and is freed a few lines after the patch content. filename is just a pointer, not a newly allocated buffer. Using /var/lock instead of /tmp sounds sane, yes.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
We're running alsactl restore on startup of an embedded system which uses a read-only rootfs. So it can't create the lockfile in the default place and hence will not restore anything.
OK, if so, /var/lock would be a better option for the default asound.state. For other cases, we can add an option to pass the lock file path.
thanks,
Takashi
-Julian
JaroslavSigned-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Tue, 06 May 2014 16:53:00 +0200, Jaroslav Kysela wrote:
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
Agreed. Above all, using a fixed path with /tmp is really fragile, easily leading to a security risk for a service that is run by root like this.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
... or give an option?
Takashi
JaroslavSigned-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
-- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Am 06/05/14 17:05, schrieb Takashi Iwai:
At Tue, 06 May 2014 16:53:00 +0200, Jaroslav Kysela wrote:
Date 6.5.2014 13:57, Julian Scheel wrote:
It can not be generally assumed that the directories in which asound.state resides are writable. Instead using /tmp as location for lock files seems more reliable.
Apart the missing free for the mallocated string and ommiting the TMPDIR environment variable, I think that the right directory for global locks is /var/lock . The default asound.state directory is now /var/lib/alsa - I don't see the benefit.
Agreed. Above all, using a fixed path with /tmp is really fragile, easily leading to a security risk for a service that is run by root like this.
I agree that /tmp is not the best choice. It was just what came to my mind first when thinking of a place where r/w access shall be possible in any system.
What's the reason for this change? Perhaps using an environmental variable to override the lock path may be more appropriate for a custom directory structure.
... or give an option?
What about using /var/lock as default, allowing to explicitly override with an option? I think this would be more correct than the current approach.
-Julian
JaroslavSigned-off-by: Julian Scheel julian@jusst.de
alsactl/lock.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/alsactl/lock.c b/alsactl/lock.c index 587a109..7ca3a09 100644 --- a/alsactl/lock.c +++ b/alsactl/lock.c @@ -36,17 +36,24 @@ static int state_lock_(const char *file, int lock, int timeout) struct flock lck; struct stat st; char lcktxt[12];
char *filename; char *nfile;
if (!do_lock) return 0;
- nfile = malloc(strlen(file) + 6);
- /* only use the actual filename, not the path */
- filename = strrchr(file, '/');
- if (!filename)
filename = file;- nfile = malloc(strlen(filename) + 10); if (nfile == NULL) { error("No enough memory..."); return -ENOMEM; }
- strcpy(nfile, file);
- strcat(nfile, ".lock");
- sprintf(nfile, "/tmp/%s.lock", filename); lck.l_type = lock ? F_WRLCK : F_UNLCK; lck.l_whence = SEEK_SET; lck.l_start = 0;
-- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (3)
-
Jaroslav Kysela -
Julian Scheel -
Takashi Iwai