| Message ID | 1399377427-23907-1-git-send-email-julian@jusst.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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. > > 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; >> > >
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 > 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; >> > >
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 > > 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; > > > > > -- > 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 >
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 > > > 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; > >> > > > > > _______________________________________________ > 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 >> >> 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; >>> >> >> >> -- >> 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 >
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;
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(-)