* Re: [PATCH] alsactl: Store lockfile in /tmp
2014-05-06 14:53 ` Jaroslav Kysela
@ 2014-05-06 14:55 ` Jaroslav Kysela
2014-05-06 15:00 ` Julian Scheel
2014-05-06 15:05 ` Takashi Iwai
2 siblings, 0 replies; 7+ messages in thread
From: Jaroslav Kysela @ 2014-05-06 14:55 UTC (permalink / raw)
To: Julian Scheel, alsa-devel
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;
>>
>
>
--
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] alsactl: Store lockfile in /tmp
2014-05-06 14:53 ` Jaroslav Kysela
2014-05-06 14:55 ` Jaroslav Kysela
@ 2014-05-06 15:00 ` Julian Scheel
2014-05-06 16:44 ` Takashi Iwai
2014-05-06 15:05 ` Takashi Iwai
2 siblings, 1 reply; 7+ messages in thread
From: Julian Scheel @ 2014-05-06 15:00 UTC (permalink / raw)
To: alsa-devel
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;
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] alsactl: Store lockfile in /tmp
2014-05-06 15:00 ` Julian Scheel
@ 2014-05-06 16:44 ` Takashi Iwai
0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2014-05-06 16:44 UTC (permalink / raw)
To: Julian Scheel; +Cc: 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] alsactl: Store lockfile in /tmp
2014-05-06 14:53 ` Jaroslav Kysela
2014-05-06 14:55 ` Jaroslav Kysela
2014-05-06 15:00 ` Julian Scheel
@ 2014-05-06 15:05 ` Takashi Iwai
2014-05-06 18:55 ` Julian Scheel
2 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2014-05-06 15:05 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Julian Scheel, 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
>
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] alsactl: Store lockfile in /tmp
2014-05-06 15:05 ` Takashi Iwai
@ 2014-05-06 18:55 ` Julian Scheel
0 siblings, 0 replies; 7+ messages in thread
From: Julian Scheel @ 2014-05-06 18:55 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela; +Cc: 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread