Re: lockdev: FTBFS on hurd-i386
Hi Svante,
On Thu, Jan 26, 2012 at 10:45:19AM +0100, Svante Signell wrote:
> static inline int
> -_dl_filename_1 (char *name,
> +_dl_filename_1 (char **name,
> const struct stat *st)
> {
(...)
> + len = strlen( LOCK_PATH) + 7 + 2*3*sizeof( int) + 1;
> + if ( (*name = malloc( len)) == NULL )
> + return -1;
> + l = sprintf( *name, "%s/LCK.%c.%03d.%03d", LOCK_PATH,
'len' is missing one character.
> static inline int
> -_dl_filename_2 (char *name,
> +_dl_filename_2 (char **name,
> const char *dev)
> {
(...)
> - for (i=strlen(LOCK_PATH)+1; name[i] != '\0'; ++i) {
> - if (name[i] == '/')
> - name[i] = ':';
(...)
> + for (i=strlen(LOCK_PATH)+1; (*name)[i] != '\0'; ++i) {
> + if ((*name)[i] == '/')
> + (*name)[i] = ':';
Misindented.
> +static char *sem_name = NULL;
> +static int oldmask= -1, len;
You only use 'len' temporarily (ie., not to store global state to be
retreived later) so it does not need to be global.
(Also it's used for a different purpose in '_dl_check_lock' --- I guess
you forgot to declare a local variable there).
[in _dl_get_semaphore]
> + len = strlen( LOCK_PATH) + 1 + strlen( SEMAPHORE) + 1;
> + if ( (sem_name = malloc( len)) == NULL )
> + return 0;
> + snprintf( sem_name, len, "%s/%s", LOCK_PATH, SEMAPHORE);
[in _dl_unlock_semaphore]
> + free( sem_name);
You'll leak the buffer if _dl_get_semaphore returns with an error,
because then you're in the "unlocked" state but a buffer still exists.
Since 'sem_name' won't change over the course of the life of the
process, I suggest you initialize it once and never free() it. I mean
something like this in _dl_get_semaphore:
if ( sem_name == NULL ) {
int len = ...
/* allocate, sprintf */
}
Also, in this function 'return 0' apparently means "success", so you
don't want to do that when malloc() fails.
[in _dl_check_lock]
> + len = strlen( LOCK_PATH) + 2 + sizeof( int) + 1;
> + if ( (tpname = malloc( len)) == NULL )
> + return 0;
> + snprintf( tpname, len, "%s/.%d", LOCK_PATH, (int)getpid());
You mean 'sizeof(int) * 3'.
> unlink( tpname); /* in case there was */
> rename( lockname, tpname);
> if ( ! (fd=fopen( tpname, "r")) ) {
| return -1;
| }
You're leaking 'tpname' here.
> @@ -528,17 +554,19 @@
> * return the pid of the owner of the lock.
> */
> /* lockfile of type /var/lock/LCK..ttyS2 */
> - _dl_filename_2( lock, p);
> - if ( (pid=_dl_check_lock( lock)) )
> + _dl_filename_2( &lock, p);
Now that '_dl_filename_*()' can return an error (-1), their return
values must be checked. (This is true for every occurence.)
> + if ( (pid=_dl_check_lock( lock)) ) {
> + free( device);
> + free( lock);
> close_n_return( pid);
> -
> + }
> /* and also check if a pid file was left around
> * do this before the static var is wiped
> * BUT! we don't fail in case the process exists, as this is not
> * a lock file, just a pid holder!
> */
> if ( pid_read ) {
> - _dl_filename_0( lock, pid_read);
> + _dl_filename_0( &lock, pid_read);
Here the old buffer 'lock' pointed to is leaked.
> _dl_check_lock( lock);
> }
>
> @@ -548,31 +576,36 @@
> * the contrary; anyway we do both tests.
> */
> /* lockfile of type /var/lock/LCK.004.064 */
> - _dl_filename_1( lock, &statbuf);
> - if ( (pid=_dl_check_lock( lock)) )
> + _dl_filename_1( &lock, &statbuf);
Likewise.
> + if ( (pid=_dl_check_lock( lock)) ) {
> + free( device);
> + free( lock);
> close_n_return( pid);
> + }
> if ( pid_read ) {
> - _dl_filename_0( lock, pid_read);
> + _dl_filename_0( &lock, pid_read);
Likewise.
> -
> +/* DONE TO HERE: srs */
Left over.
> @@ -588,16 +621,22 @@
> oldmask = umask( 0); /* give full permissions to files created */
> if ( ! (p=_dl_check_devname( devname)) )
> close_n_return( -1);
> - strcpy( device, DEV_PATH);
> - strcat( device, p); /* now device has a copy of the pathname */
> +
> + /* FIXME strlen(p)?? */
I'm not sure what you mean.
> + len = strlen( DEV_PATH) + strlen( p) + 1;
> + if ( (device = malloc( len)) == NULL )
> + close_n_return(0);
0 would mean success.
(there's another one I first missed in 'dev_testlock')
> @@ -622,14 +663,16 @@
> * return the pid of the owner of the lock.
> */
> /* lockfile of type /var/lock/LCK..ttyS2 */
> - _dl_filename_2( lock2, p);
> + _dl_filename_2( &lock2, p);
> pid = _dl_check_lock( lock2);
> if ( pid && pid != our_pid ) {
> unlink( lock0);
> + free( lock0);
> +
free(lock2) as well.
> close_n_return( pid); /* error or locked by someone else */
> }
> if ( pid_read ) { /* modifyed by _dl_check_lock() */
> - _dl_filename_0( lock, pid_read);
> + _dl_filename_0( &lock, pid_read);
Previous value leaked.
> _dl_check_lock( lock); /* remove stale pid file */
> }
> /* not locked or already owned by us */
> @@ -638,20 +681,23 @@
> * lock happens
> */
> /* lockfile of type /var/lock/LCK.004.064 */
> - _dl_filename_1( lock1, &statbuf);
> + _dl_filename_1( &lock1, &statbuf);
> while ( ! (pid=_dl_check_lock( lock1)) ) {
> if (( link( lock0, lock1) == -1 ) && ( errno != EEXIST )) {
> unlink( lock0);
> + free( lock0);
> + free( lock1);
free(lock2) as well.
> close_n_return( -1);
> }
> }
> if ( pid != our_pid ) {
> /* error or another one owns it now */
> unlink( lock0);
> + free( lock0);
free(lock1, lock2) as well.
> close_n_return( pid);
> }
> if ( pid_read ) { /* modifyed by _dl_check_lock() */
> - _dl_filename_0( lock, pid_read);
> + _dl_filename_0( &lock, pid_read);
> _dl_check_lock( lock); /* remove stale pid file */
free(lock)
> }
> /* from this point lock1 is OUR! */
> @@ -661,6 +707,8 @@
> if (( link( lock0, lock2) == -1 ) && ( errno != EEXIST )) {
> unlink( lock0);
> unlink( lock1);
> + free( lock0);
> + free( lock1);
free(lock2) as well.
> close_n_return( -1);
> }
> }
> @@ -672,11 +720,13 @@
> */
> unlink( lock0);
> unlink( lock1);
> + free( lock0);
> + free( lock1);
free(lock2) as well.
> close_n_return( pid);
> }
> /* quite unlike, but ... */
> if ( pid_read ) { /* modifyed by _dl_check_lock() */
> - _dl_filename_0( lock, pid_read);
> + _dl_filename_0( &lock, pid_read);
> _dl_check_lock( lock); /* remove stale pid file */
free(lock).
> }
>
> @@ -694,15 +744,19 @@
> pid2 = _dl_check_lock( lock2);
> if (( pid == pid2 ) && ( pid == our_pid )) {
> _debug( 2, "dev_lock() got lock\n");
> + free( lock1);
> + free( lock2);
free(lock0) as well.
> close_n_return( 0); /* locked by us */
> }
> /* oh, no! someone else stepped in! */
> if ( pid == our_pid ) {
> unlink( lock1);
> + free( lock1);
Not here, because..
> pid = 0;
> }
> if ( pid2 == our_pid ) {
> unlink( lock2);
> + free( lock2);
Not here, because..
> pid2 = 0;
> }
> if ( pid && pid2 ) {
> @@ -710,6 +764,9 @@
> _debug( 1, "dev_lock() process %d owns file %s\n", (int)pid, lock1);
> _debug( 1, "dev_lock() process %d owns file %s\n", (int)pid2, lock2);
> _debug( 1, "dev_lock() process %d (we) have no lock!\n", (int)our_pid);
> + free( lock0);
> + free( lock1);
> + free( lock2);
..'lock2' could have been freed 3 times by now..
> close_n_return( -1);
> }
> close_n_return( (pid + pid2));
..or none at all.
> @@ -721,13 +778,14 @@
> dev_relock (const char *devname,
> const pid_t old_pid)
> {
> - const char * p;
> + const char *p;
Gratuitous change.
> - char device[MAXPATHLEN+1];
> - char lock1[MAXPATHLEN+1];
> - char lock2[MAXPATHLEN+1];
> + char *device;
Inconsistent spacing.
> + char * lock1;
> + char * lock2;
> struct stat statbuf;
> pid_t pid, our_pid;
> FILE *fd = 0;
> + int len;
>
> #if DEBUG
> if ( env_var_debug == -1 ) {
> @@ -743,14 +801,17 @@
> oldmask = umask( 0); /* give full permissions to files created */
> if ( ! (p=_dl_check_devname( devname)) )
> close_n_return( -1);
> - strcpy( device, DEV_PATH);
> - strcat( device, p); /* now device has a copy of the pathname */
> + len = strlen( DEV_PATH) + strlen( p) + 1;
> + if ( (device = malloc( len)) == NULL )
> + close_n_return(0);
0 means success.
> + snprintf( device, len, "%s%s", DEV_PATH, p); /* now device has a copy of the pathname */
> _debug( 2, "dev_relock() device = %s\n", device);
>
> /* check the device name for existence and retrieve the major
> * and minor numbers
> */
> if ( stat( device, &statbuf) == -1 ) {
> + free( device);
> close_n_return( -1);
> }
'device' is no longer used, 'free(device)' here.
>
> @@ -763,37 +824,52 @@
> * return the pid of the owner of the lock.
> */
> /* lockfile of type /var/lock/LCK..ttyS2 */
> - _dl_filename_2( lock2, p);
> + _dl_filename_2( &lock2, p);
> pid = _dl_check_lock( lock2);
> - if ( pid && old_pid && pid != old_pid )
> + if ( pid && old_pid && pid != old_pid ) {
> + free( device);
> + free( lock2);
> close_n_return( pid); /* error or locked by someone else */
> -
> + }
> /* lockfile of type /var/lock/LCK.004.064 */
> - _dl_filename_1( lock1, &statbuf);
> + _dl_filename_1( &lock1, &statbuf);
> pid = _dl_check_lock( lock1);
> - if ( pid && old_pid && pid != old_pid )
> + if ( pid && old_pid && pid != old_pid ) {
> + free( device);
> + free( lock1);
free(lock2)
> close_n_return( pid); /* error or locked by someone else */
> -
> + }
> if ( ! pid ) { /* not locked ??? */
> /* go and lock it */
> + free( device);
> + free( lock1);
free(lock2)
> return( dev_lock( devname));
> }
>
> /* we don't rewrite the pids in the lock files untill we're sure
> * we own all the lockfiles
> */
> - if ( ! (fd=fopen( lock1, "w")) )
> + if ( ! (fd=fopen( lock1, "w")) ) {
> + free( device);
> + free( lock1);
free(lock2)
> @@ -828,14 +905,18 @@
> oldmask = umask( 0); /* give full permissions to files created */
> if ( ! (p=_dl_check_devname( devname)) )
> close_n_return( -1);
> - strcpy( device, DEV_PATH);
> - strcat( device, p); /* now device has a copy of the pathname */
> + /* FIXME strlen(p)?? */
Still not sure what you mean.
> + len = strlen( DEV_PATH) + strlen( p) + 1;
> + if ( (device = malloc( len)) == NULL )
> + close_n_return(0);
> + snprintf( device, len, "%s%s", DEV_PATH, p); /* now device has a copy of the pathname */
> _debug( 2, "dev_unlock() device = %s\n", device);
'device' is no longer used, you should free it here.
>
> /* check the device name for existence and retrieve the major
> * and minor numbers
> */
> if ( stat( device, &statbuf) == -1 ) {
> + free( device);
> close_n_return( -1);
> }
>
> @@ -844,26 +925,35 @@
> * return the pid of the owner of the lock.
> */
> /* lockfile of type /var/lock/LCK..ttyS2 */
> - _dl_filename_2( lock2, p);
> + _dl_filename_2( &lock2, p);
> wpid = _dl_check_lock( lock2);
> - if ( pid && wpid && pid != wpid )
> + if ( pid && wpid && pid != wpid ) {
> + free( device);
> + free( lock2);
> close_n_return( wpid); /* error or locked by someone else */
> -
> + }
> /* lockfile of type /var/lock/LCK.004.064 */
> - _dl_filename_1( lock1, &statbuf);
> + _dl_filename_1( &lock1, &statbuf);
> wpid = _dl_check_lock( lock1);
> - if ( pid && wpid && pid != wpid )
> + if ( pid && wpid && pid != wpid ) {
> + free( device);
> + free( lock1);
free(lock2)
> close_n_return( wpid); /* error or locked by someone else */
> -
> - _dl_filename_0( lock0, wpid);
> - if ( wpid == _dl_check_lock( lock0))
> + }
> + _dl_filename_0( &lock0, wpid);
> + if ( wpid == _dl_check_lock( lock0)) {
> unlink( lock0);
> + free( lock0);
Not here.
> + }
>
> /* anyway now we remove the files, in the reversed order than
> * they have been built.
> */
> + free( device);
> unlink( lock2);
> unlink( lock1);
> + free( lock2);
> + free( lock1);
free(lock0)
> _debug( 2, "dev_unlock() unlocked\n");
> close_n_return( 0); /* successfully unlocked */
> }
Good night, :-)
--
Jeremie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org
Reply to: