Prevent winbind idmap corruption

[Andrew Bartlett]

On Thu, 2002-12-19 at 02:22, Michael Steffens wrote:
> Hi,
> 
> the attached patch prevents winbindd from corrupting the
> id mapping database in case of write failure. For example when
> the filesystem hosting the TDB file is full.
> 
> Storing a new meapping consists of three steps
> 
>   1. allocate UID/GID (increment HWM)
>   2. store mapping UID/GID : SID
>   3. store reverse mapping SID : UID/GID
> 
> which should be done as a transaction, either completely or not
> at all.
> 
> The present winbindd_idmap.c does not check success of the
> operations above, and will result in an inconsistent mapping
> database when any of them fails.
> 
> The patched version does check success, and rolls back in
> case of failure.
> 
> It's not 100% failure proof (transaction is not atomic), but
> better than before IMO. :)

This looks like a good idea - getting this stuff right does matter...

More comments below:

> Michael
> 
> ----
> 

> Index: nsswitch/winbindd_idmap.c
> ===================================================================
> RCS file: /cvsroot/samba/source/nsswitch/winbindd_idmap.c,v
> retrieving revision 1.3.4.13
> diff -u -r1.3.4.13 winbindd_idmap.c
> --- nsswitch/winbindd_idmap.c	27 Apr 2002 03:04:08 -0000	1.3.4.13
> +++ nsswitch/winbindd_idmap.c	18 Dec 2002 14:51:08 -0000
> @@ -44,6 +44,8 @@
>  
>      if ((hwm = tdb_fetch_int32(idmap_tdb, 
>                               isgroup ? HWM_GROUP : HWM_USER)) == -1) {
> +        DEBUG(0, ("Failed to fetch %s : %s\n", isgroup ? HWM_GROUP : HWM_USER,
> +            tdb_errorstr(idmap_tdb)));
>          return False;
>      }
>  
> @@ -63,7 +65,45 @@
>  
>      /* Store new high water mark */
>  
> -    tdb_store_int32(idmap_tdb, isgroup ? HWM_GROUP : HWM_USER, hwm);
> +    if (tdb_store_int32(idmap_tdb, isgroup ? HWM_GROUP : HWM_USER, hwm)) {
> +        DEBUG(0, ("Failed to store %s %d : %s\n", isgroup ? HWM_GROUP : HWM_USER,
> +            hwm, tdb_errorstr(idmap_tdb)));
> +        return False;
> +    }
> +
> +    return True;
> +}
> +
> +/* Deallocate either a user or group id, used for failure rollback */
> +
> +static BOOL deallocate_id(uid_t id, BOOL isgroup)

I don't like the use of uid_t for gid_t, on the assumption that they
must be the same...  I know it will never happen, but it just seems the
wrong way to go about it.  As you assume an int32 below, then maybe make
it an int32 in the paramaters.

However, why do we need to 'roll back' the High Water Mark at all?

Instead, I would avoid the extra operation (if something failed already,
then don't risk that something else could fail too) and just leave that
uid/gid 'dead'.

Andrew Bartlett

> +{
> +    int hwm;
> +
> +    /* Get current high water mark */
> +
> +    if ((hwm = tdb_fetch_int32(idmap_tdb, 
> +                             isgroup ? HWM_GROUP : HWM_USER)) == -1) {
> +        DEBUG(0, ("Failed to fetch %s : %s\n", isgroup ? HWM_GROUP : HWM_USER,
> +            tdb_errorstr(idmap_tdb)));
> +        return False;
> +    }
> +
> +    if (hwm != id + 1) {
> +        /* Should actually never happen, internal redundancy... */
> +        DEBUG(0, ("winbind %s mismatch on deallocation!\n", isgroup ? HWM_GROUP : HWM_USER));
> +        return False;
> +    }
> +
> +    hwm--;
> +
> +    /* Store new high water mark */
> +
> +    if (tdb_store_int32(idmap_tdb, isgroup ? HWM_GROUP : HWM_USER, hwm)) {
> +        DEBUG(0, ("Failed to store %s %d : %s\n", isgroup ? HWM_GROUP : HWM_USER,
> +           hwm, tdb_errorstr(idmap_tdb)));
> +        return False;
> +    }
>  
>      return True;
>  }
> @@ -109,16 +149,37 @@
>              fstring keystr2;
>  
>              /* Store new id */
> -            
> +
>              slprintf(keystr2, sizeof(keystr2), "%s %d", isgroup ? "GID" : "UID", *id);
>  
>              data.dptr = keystr2;
>              data.dsize = strlen(keystr2) + 1;
>  
> -            tdb_store(idmap_tdb, key, data, TDB_REPLACE);
> -            tdb_store(idmap_tdb, data, key, TDB_REPLACE);
> +            /* If any of the following actions fails try to
> +               revert modifications successfully made so far. */
>  
>              result = True;
> +
> +            if (result && tdb_store(idmap_tdb, key, data, TDB_REPLACE)) {
> +                DEBUG(0, ("Failed to store id mapping %s:%s : %s\n",
> +                          key.dptr, data.dptr, tdb_errorstr(idmap_tdb)));
> +
> +                if (!deallocate_id(*id, isgroup))
> +                    DEBUG(0, ("Failed to rollback id mapping\n"));
> +
> +                result = False;
> +            }
> +
> +            if (result && tdb_store(idmap_tdb, data, key, TDB_REPLACE)) {
> +                DEBUG(0, ("Failed to store reverse id mapping %s:%s : %s\n",
> +                          data.dptr, key.dptr, tdb_errorstr(idmap_tdb)));
> +
> +                if (!deallocate_id(*id, isgroup) || tdb_delete(idmap_tdb, key))
> +                    DEBUG(0, ("Failed to rollback id mapping\n"));
> +
> +                tdb_delete(idmap_tdb, key);
> +                result = False;
> +            }
>          }
>      }
>  
-- 
Andrew Bartlett                                 abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team  abartlet at samba.org
Student Network Administrator, Hawker College   abartlet at hawkerc.net
http://samba.org     http://build.samba.org     http://hawkerc.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20021229/899d60c9/attachment.bin