[PATCH] alsactl: Store lockfile in /tmp

[PATCH] alsactl: Store lockfile in /tmp

[Julian Scheel]

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;
-- 
1.9.2

Re: [PATCH] alsactl: Store lockfile in /tmp

[Jaroslav Kysela]

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;
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH] alsactl: Store lockfile in /tmp

[Jaroslav Kysela]

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.

Re: [PATCH] alsactl: Store lockfile in /tmp

[Julian Scheel]

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;
>>
>
>

Re: [PATCH] alsactl: Store lockfile in /tmp

[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.

> 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
> 

Re: [PATCH] alsactl: Store lockfile in /tmp

[Takashi Iwai]

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
> 

Re: [PATCH] alsactl: Store lockfile in /tmp

[Julian Scheel]

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
>