more verbose checking in talloc and util_pw

fixed tdbsam memory corruption (and segfault)
reducing calls to pdb_uid_to_user_rid and countrary to 0 to move to a non alghoritmic rid allocation with some passdb modules.

diff --git a/source/lib/talloc.c b/source/lib/talloc.c
index 543165c8819fd983c2e10a9c6776aad834d18c3e..6ac784a929754680b95bd1c2d58c8c5c632c1fca 100644 (file)
--- a/source/lib/talloc.c
+++ b/source/lib/talloc.c
@@ -121,13 +121,13 @@ TALLOC_CTX *talloc_init(void)
 {
        TALLOC_CTX *t;
 
-       t = (TALLOC_CTX *)malloc(sizeof(*t));
-       if (!t) return NULL;
-
-       t->list = NULL;
-       t->total_alloc_size = 0;
-       t->name = NULL;
-       talloc_enroll(t);
+       t = (TALLOC_CTX *)malloc(sizeof(TALLOC_CTX));
+       if (t) {
+               t->list = NULL;
+               t->total_alloc_size = 0;
+               t->name = NULL;
+               talloc_enroll(t);
+       }
 
        return t;
 }
@@ -144,7 +144,7 @@ TALLOC_CTX *talloc_init(void)
        va_list ap;
 
        t = talloc_init();
-       if (fmt) {
+       if (t && fmt) {
                va_start(ap, fmt);
                t->name = talloc_vasprintf(t, fmt, ap);
                va_end(ap);
@@ -160,23 +160,22 @@ void *talloc(TALLOC_CTX *t, size_t size)
        void *p;
        struct talloc_chunk *tc;
 
-       if (size == 0) return NULL;
+       if (!t || size == 0) return NULL;
 
        p = malloc(size);
-       if (!p) return p;
-
-       tc = malloc(sizeof(*tc));
-       if (!tc) {
-               SAFE_FREE(p);
-               return NULL;
+       if (p) {
+               tc = malloc(sizeof(*tc));
+               if (tc) {
+                       tc->ptr = p;
+                       tc->size = size;
+                       tc->next = t->list;
+                       t->list = tc;
+                       t->total_alloc_size += size;
+               }
+               else {
+                       SAFE_FREE(p);
+               }
        }
-
-       tc->ptr = p;
-       tc->size = size;
-       tc->next = t->list;
-       t->list = tc;
-       t->total_alloc_size += size;
-
        return p;
 }
 
@@ -187,7 +186,7 @@ void *talloc_realloc(TALLOC_CTX *t, void *ptr, size_t size)
        void *new_ptr;
 
        /* size zero is equivalent to free() */
-       if (size == 0)
+       if (!t || size == 0)
                return NULL;
 
        /* realloc(NULL) is equavalent to malloc() */
@@ -232,21 +231,28 @@ void talloc_destroy(TALLOC_CTX *t)
 {
        if (!t)
                return;
+
        talloc_destroy_pool(t);
        talloc_disenroll(t);
-       memset(t, 0, sizeof(*t));
+       memset(t, 0, sizeof(TALLOC_CTX));
        SAFE_FREE(t);
 }
 
 /** Return the current total size of the pool. */
 size_t talloc_pool_size(TALLOC_CTX *t)
 {
-       return t->total_alloc_size;
+       if (t)
+               return t->total_alloc_size;
+       else
+               return 0;
 }
 
 const char * talloc_pool_name(TALLOC_CTX const *t)
 {
-       return t->name;
+       if (t)
+               return t->name;
+       else
+               return NULL;
 }
 
 
@@ -266,10 +272,8 @@ void *talloc_memdup(TALLOC_CTX *t, const void *p, size_t size)
 {
        void *newp = talloc(t,size);
 
-       if (!newp)
-               return 0;
-
-       memcpy(newp, p, size);
+       if (newp)
+               memcpy(newp, p, size);
 
        return newp;
 }
@@ -277,7 +281,10 @@ void *talloc_memdup(TALLOC_CTX *t, const void *p, size_t size)
 /** strdup with a talloc */
 char *talloc_strdup(TALLOC_CTX *t, const char *p)
 {
-       return talloc_memdup(t, p, strlen(p) + 1);
+       if (p)
+               return talloc_memdup(t, p, strlen(p) + 1);
+       else
+               return NULL;
 }
 
 /**
@@ -304,9 +311,8 @@ char *talloc_strdup(TALLOC_CTX *t, const char *p)
        len = vsnprintf(NULL, 0, fmt, ap);
 
        ret = talloc(t, len+1);
-       if (!ret) return NULL;
-
-       vsnprintf(ret, len+1, fmt, ap);
+       if (ret)
+               vsnprintf(ret, len+1, fmt, ap);
 
        return ret;
 }
@@ -363,6 +369,8 @@ char *talloc_describe_all(TALLOC_CTX *rt)
        TALLOC_CTX *it;
        char *s;
 
+       if (!rt) return NULL;
+
        s = talloc_asprintf(rt, "global talloc allocations in pid: %u\n",
                            (unsigned) sys_getpid());
        s = talloc_asprintf_append(rt, s, "%-40s %8s %8s\n",
@@ -418,12 +426,14 @@ void talloc_get_allocation(TALLOC_CTX *t,
 {
        struct talloc_chunk *chunk;
 
-       *total_bytes = 0;
-       *n_chunks = 0;
+       if (t) {
+               *total_bytes = 0;
+               *n_chunks = 0;
 
-       for (chunk = t->list; chunk; chunk = chunk->next) {
-               n_chunks[0]++;
-               *total_bytes += chunk->size;
+               for (chunk = t->list; chunk; chunk = chunk->next) {
+                       n_chunks[0]++;
+                       *total_bytes += chunk->size;
+               }
        }
 }
 

diff --git a/source/lib/util_pw.c b/source/lib/util_pw.c
index 67ed43f216d4853b0e3f452d5f532171669b5a06..259649a064e210a744723912ea89f5b6f8933bd5 100644 (file)
--- a/source/lib/util_pw.c
+++ b/source/lib/util_pw.c
@@ -68,7 +68,7 @@ struct passwd *make_modifyable_passwd(const struct passwd *from)
 
 static struct passwd *alloc_copy_passwd(const struct passwd *from) 
 {
-       struct passwd *ret = smb_xmalloc(sizeof(*ret));
+       struct passwd *ret = smb_xmalloc(sizeof(struct passwd));
        ZERO_STRUCTP(ret);
        ret->pw_name = smb_xstrdup(from->pw_name);
        ret->pw_passwd = smb_xstrdup(from->pw_passwd);

diff --git a/source/passdb/pdb_get_set.c b/source/passdb/pdb_get_set.c
index 181364ab6ba2ad85be354a75dcade713215d2637..cf77efd38fec1eec1c9ec2a3e46daa53d70c1af1 100644 (file)
--- a/source/passdb/pdb_get_set.c
+++ b/source/passdb/pdb_get_set.c
@@ -493,11 +493,11 @@ BOOL pdb_set_username(SAM_ACCOUNT *sampass, const char *username)
 {      
        if (!sampass)
                return False;
-
-       DEBUG(10, ("pdb_set_username: setting username %s, was %s\n", 
-                  username, sampass->private.username));
  
        if (username) { 
+               DEBUG(10, ("pdb_set_username: setting username %s, was %s\n", username,
+                       (sampass->private.username)?(sampass->private.username):"NULL"));
+
                sampass->private.username = talloc_strdup(sampass->mem_ctx, username);
 
                if (!sampass->private.username) {
@@ -521,10 +521,10 @@ BOOL pdb_set_domain(SAM_ACCOUNT *sampass, const char *domain)
        if (!sampass)
                return False;
 
-       DEBUG(10, ("pdb_set_domain: setting domain %s, was %s\n", 
-                  domain, sampass->private.domain));
- 
        if (domain) { 
+               DEBUG(10, ("pdb_set_domain: setting domain %s, was %s\n", domain,
+                       (sampass->private.domain)?(sampass->private.domain):"NULL"));
+
                sampass->private.domain = talloc_strdup(sampass->mem_ctx, domain);
 
                if (!sampass->private.domain) {
@@ -548,10 +548,10 @@ BOOL pdb_set_nt_username(SAM_ACCOUNT *sampass, const char *nt_username)
        if (!sampass)
                return False;
 
-       DEBUG(10, ("pdb_set_nt_username: setting nt username %s, was %s\n", 
-                  nt_username, sampass->private.nt_username));
- 
        if (nt_username) { 
+               DEBUG(10, ("pdb_set_nt_username: setting nt username %s, was %s\n", nt_username,
+                       (sampass->private.nt_username)?(sampass->private.nt_username):"NULL"));
+ 
                sampass->private.nt_username = talloc_strdup(sampass->mem_ctx, nt_username);
                
                if (!sampass->private.nt_username) {
@@ -575,10 +575,10 @@ BOOL pdb_set_fullname(SAM_ACCOUNT *sampass, const char *full_name)
        if (!sampass)
                return False;
 
-       DEBUG(10, ("pdb_set_full_name: setting full name %s, was %s\n", 
-                  full_name, sampass->private.full_name));
-       
        if (full_name) { 
+               DEBUG(10, ("pdb_set_full_name: setting full name %s, was %s\n", full_name,
+                       (sampass->private.full_name)?(sampass->private.full_name):"NULL"));
+       
                sampass->private.full_name = talloc_strdup(sampass->mem_ctx, full_name);
 
                if (!sampass->private.full_name) {
@@ -602,10 +602,10 @@ BOOL pdb_set_logon_script(SAM_ACCOUNT *sampass, const char *logon_script, BOOL s
        if (!sampass)
                return False;
 
-       DEBUG(10, ("pdb_set_logon_script: setting logon script (store:%d) %s, was %s\n", 
-                  store, logon_script, sampass->private.logon_script));
- 
        if (logon_script) { 
+               DEBUG(10, ("pdb_set_logon_script: setting logon script %s, was %s\n", logon_script,
+                       (sampass->private.logon_script)?(sampass->private.logon_script):"NULL"));
+ 
                sampass->private.logon_script = talloc_strdup(sampass->mem_ctx, logon_script);
 
                if (!sampass->private.logon_script) {
@@ -617,8 +617,10 @@ BOOL pdb_set_logon_script(SAM_ACCOUNT *sampass, const char *logon_script, BOOL s
                sampass->private.logon_script = PDB_NOT_QUITE_NULL;
        }
        
-       if (store)
-               pdb_set_init_flag(sampass, FLAG_SAM_LOGONSCRIPT); 
+       if (store) {
+               DEBUG(10, ("pdb_set_logon_script: setting logon script sam flag!"));
+               pdb_set_init_flag(sampass, FLAG_SAM_LOGONSCRIPT);
+       }
 
        return True;
 }
@@ -632,10 +634,10 @@ BOOL pdb_set_profile_path (SAM_ACCOUNT *sampass, const char *profile_path, BOOL
        if (!sampass)
                return False;
 
-       DEBUG(10, ("pdb_set_profile_path: setting profile path (store:%d) %s, was %s\n", 
-                  store, profile_path, sampass->private.profile_path));
- 
        if (profile_path) { 
+               DEBUG(10, ("pdb_set_profile_path: setting profile path %s, was %s\n", profile_path,
+                       (sampass->private.profile_path)?(sampass->private.profile_path):"NULL"));
+ 
                sampass->private.profile_path = talloc_strdup(sampass->mem_ctx, profile_path);
                
                if (!sampass->private.profile_path) {
@@ -647,8 +649,10 @@ BOOL pdb_set_profile_path (SAM_ACCOUNT *sampass, const char *profile_path, BOOL
                sampass->private.profile_path = PDB_NOT_QUITE_NULL;
        }
 
-       if (store)
+       if (store) {
+               DEBUG(10, ("pdb_set_profile_path: setting profile path sam flag!"));
                pdb_set_init_flag(sampass, FLAG_SAM_PROFILE);
+       }
 
        return True;
 }
@@ -663,6 +667,9 @@ BOOL pdb_set_dir_drive (SAM_ACCOUNT *sampass, const char *dir_drive, BOOL store)
                return False;
 
        if (dir_drive) { 
+               DEBUG(10, ("pdb_set_dir_drive: setting dir drive %s, was %s\n", dir_drive,
+                       (sampass->private.dir_drive)?(sampass->private.dir_drive):"NULL"));
+ 
                sampass->private.dir_drive = talloc_strdup(sampass->mem_ctx, dir_drive);
                
                if (!sampass->private.dir_drive) {
@@ -674,8 +681,10 @@ BOOL pdb_set_dir_drive (SAM_ACCOUNT *sampass, const char *dir_drive, BOOL store)
                sampass->private.dir_drive = PDB_NOT_QUITE_NULL;
        }
        
-       if (store)
+       if (store) {
+               DEBUG(10, ("pdb_set_dir_drive: setting dir drive sam flag!"));
                pdb_set_init_flag(sampass, FLAG_SAM_DRIVE);
+       }
 
        return True;
 }
@@ -690,6 +699,9 @@ BOOL pdb_set_homedir (SAM_ACCOUNT *sampass, const char *home_dir, BOOL store)
                return False;
 
        if (home_dir) { 
+               DEBUG(10, ("pdb_set_homedir: setting home dir %s, was %s\n", home_dir,
+                       (sampass->private.home_dir)?(sampass->private.home_dir):"NULL"));
+ 
                sampass->private.home_dir = talloc_strdup(sampass->mem_ctx, home_dir);
                
                if (!sampass->private.home_dir) {
@@ -701,8 +713,10 @@ BOOL pdb_set_homedir (SAM_ACCOUNT *sampass, const char *home_dir, BOOL store)
                sampass->private.home_dir = PDB_NOT_QUITE_NULL;
        }
 
-       if (store)
+       if (store) {
+               DEBUG(10, ("pdb_set_homedir: setting home dir sam flag!"));
                pdb_set_init_flag(sampass, FLAG_SAM_SMBHOME);
+       }
 
        return True;
 }
@@ -741,6 +755,9 @@ BOOL pdb_set_workstations (SAM_ACCOUNT *sampass, const char *workstations)
                return False;
 
        if (workstations) { 
+               DEBUG(10, ("pdb_set_workstations: setting workstations %s, was %s\n", workstations,
+                       (sampass->private.workstations)?(sampass->private.workstations):"NULL"));
+ 
                sampass->private.workstations = talloc_strdup(sampass->mem_ctx, workstations);
 
                if (!sampass->private.workstations) {
@@ -787,6 +804,7 @@ BOOL pdb_set_munged_dial (SAM_ACCOUNT *sampass, const char *munged_dial)
 {
        if (!sampass)
                return False;
+
        if (munged_dial) { 
                sampass->private.munged_dial = talloc_strdup(sampass->mem_ctx, munged_dial);
                

diff --git a/source/passdb/pdb_tdb.c b/source/passdb/pdb_tdb.c
index 86089cfd692c34c12937d61df65e14fb08e15e73..b55a74d29095e52c47559351a986a2647405e0b1 100644 (file)
--- a/source/passdb/pdb_tdb.c
+++ b/source/passdb/pdb_tdb.c
@@ -90,6 +90,7 @@ static BOOL init_sam_from_buffer (struct tdbsam_privates *tdb_state,
        BOOL ret = True;
        BOOL setflag;
        gid_t gid = -1; /* This is what standard sub advanced expects if no gid is known */
+       pstring sub_buffer;
        
        if(sampass == NULL || buf == NULL) {
                DEBUG(0, ("init_sam_from_buffer: NULL parameters found!\n"));
@@ -144,9 +145,8 @@ static BOOL init_sam_from_buffer (struct tdbsam_privates *tdb_state,
                 * getpwnam() is used instead of Get_Pwnam() as we do not need
                 * to try case permutations
                 */
-               if (!username || !(pw=getpwnam_alloc(username))) {
-                       DEBUG(0,("tdbsam: getpwnam_alloc(%s) return NULL.  User does not exist!\n", 
-                                 username?username:"NULL"));
+               if (!username || !(pw = getpwnam_alloc(username))) {
+                       DEBUG(0,("tdbsam: getpwnam_alloc(%s) return NULL.  User does not exist!\n", username?username:"NULL"));
                        ret = False;
                        goto done;
                }
@@ -174,9 +174,11 @@ static BOOL init_sam_from_buffer (struct tdbsam_privates *tdb_state,
        if (homedir) setflag = True;
        else {
                setflag = False;
-               homedir = strdup(lp_logon_home());
+               pstrcpy(sub_buffer, lp_logon_home());
+               /* standard_sub_advanced() assumes pstring is passed!! */
+               standard_sub_advanced(-1, username, "", gid, username, sub_buffer);
+               homedir = strdup(sub_buffer);
                if(!homedir) { ret = False; goto done; }
-               standard_sub_advanced(-1, username, "", gid, username, homedir);
                DEBUG(5,("Home directory set back to %s\n", homedir));
        }
        pdb_set_homedir(sampass, homedir, setflag);
@@ -184,30 +186,33 @@ static BOOL init_sam_from_buffer (struct tdbsam_privates *tdb_state,
        if (dir_drive) setflag = True;
        else {
                setflag = False;
-               dir_drive = strdup(lp_logon_drive());
+               pstrcpy(sub_buffer, lp_logon_drive());
+               standard_sub_advanced(-1, username, "", gid, username, sub_buffer);
+               dir_drive = strdup(sub_buffer);
                if(!dir_drive) { ret = False; goto done; }
-               standard_sub_advanced(-1, username, "", gid, username, dir_drive);
-               DEBUG(5,("Home directory set back to %s\n", dir_drive));
+               DEBUG(5,("Drive set back to %s\n", dir_drive));
        }
        pdb_set_dir_drive(sampass, dir_drive, setflag);
 
        if (logon_script) setflag = True;
        else {
                setflag = False;
-               logon_script = strdup(lp_logon_script());
+               pstrcpy(sub_buffer, lp_logon_script());
+               standard_sub_advanced(-1, username, "", gid, username, sub_buffer);
+               logon_script = strdup(sub_buffer);
                if(!logon_script) { ret = False; goto done; }
-               standard_sub_advanced(-1, username, "", gid, username, logon_script);
-               DEBUG(5,("Home directory set back to %s\n", logon_script));
+               DEBUG(5,("Logon script set back to %s\n", logon_script));
        }
        pdb_set_logon_script(sampass, logon_script, setflag);
 
        if (profile_path) setflag = True;
        else {
                setflag = False;
-               profile_path = strdup(lp_logon_path());
+               pstrcpy(sub_buffer, lp_logon_path());
+               standard_sub_advanced(-1, username, "", gid, username, sub_buffer);
+               profile_path = strdup(sub_buffer);
                if(!profile_path) { ret = False; goto done; }
-               standard_sub_advanced(-1, username, "", gid, username, profile_path);
-               DEBUG(5,("Home directory set back to %s\n", profile_path));
+               DEBUG(5,("Profile path set back to %s\n", profile_path));
        }
        pdb_set_profile_path(sampass, profile_path, setflag);
 
@@ -223,8 +228,6 @@ static BOOL init_sam_from_buffer (struct tdbsam_privates *tdb_state,
                goto done;
        }
 
-       /*pdb_set_uid(sampass, uid);
-       pdb_set_gid(sampass, gid);*/
        pdb_set_user_rid(sampass, user_rid);
        pdb_set_group_rid(sampass, group_rid);
        pdb_set_unknown_3(sampass, unknown_3);

diff --git a/source/rpc_server/srv_samr_nt.c b/source/rpc_server/srv_samr_nt.c
index eb8ec16f45ee2357e6c4bd841a05a57a2bccae8e..542e4796c284b3f64ba9cfc33a51bb770ec87388 100644 (file)
--- a/source/rpc_server/srv_samr_nt.c
+++ b/source/rpc_server/srv_samr_nt.c
@@ -2835,6 +2835,9 @@ NTSTATUS _samr_add_aliasmem(pipes_struct *p, SAMR_Q_ADD_ALIASMEM *q_u, SAMR_R_AD
        fstring grp_name;
        uint32 rid;
        GROUP_MAP map;
+       NTSTATUS ret;
+       SAM_ACCOUNT *sam_user;
+       BOOL check;
 
        /* Find the policy handle. Open a policy on it. */
        if (!get_lsa_policy_samr_sid(p, &q_u->alias_pol, &alias_sid)) 
@@ -2859,7 +2862,23 @@ NTSTATUS _samr_add_aliasmem(pipes_struct *p, SAMR_Q_ADD_ALIASMEM *q_u, SAMR_R_AD
        }
 
        sid_split_rid(&q_u->sid.sid, &rid);
-       uid=pdb_user_rid_to_uid(rid);
+       
+       ret = pdb_init_sam(&sam_user);
+       if (NT_STATUS_IS_ERR(ret))
+               return ret;
+       
+       become_root();
+       check = pdb_getsampwrid(sam_user, rid);
+       unbecome_root();
+       
+       if (check != True)
+               return NT_STATUS_NO_SUCH_USER;
+       
+       uid = pdb_get_uid(sam_user);
+       if (uid == -1)
+               return NT_STATUS_NO_SUCH_USER;
+
+       pdb_free_sam(&sam_user);
 
        if ((pwd=getpwuid(uid)) == NULL)
                return NT_STATUS_NO_SUCH_USER;
@@ -2963,6 +2982,10 @@ NTSTATUS _samr_add_groupmem(pipes_struct *p, SAMR_Q_ADD_GROUPMEM *q_u, SAMR_R_AD
        struct group *grp;
        fstring grp_name;
        GROUP_MAP map;
+       uid_t uid;
+       NTSTATUS ret;
+       SAM_ACCOUNT *sam_user;
+       BOOL check;
 
        /* Find the policy handle. Open a policy on it. */
        if (!get_lsa_policy_samr_sid(p, &q_u->pol, &group_sid)) 
@@ -2979,7 +3002,24 @@ NTSTATUS _samr_add_groupmem(pipes_struct *p, SAMR_Q_ADD_GROUPMEM *q_u, SAMR_R_AD
        if(!get_domain_group_from_sid(group_sid, &map, MAPPING_WITHOUT_PRIV))
                return NT_STATUS_NO_SUCH_GROUP;
 
-       if ((pwd=getpwuid(pdb_user_rid_to_uid(q_u->rid))) ==NULL)
+       ret = pdb_init_sam(&sam_user);
+       if (NT_STATUS_IS_ERR(ret))
+               return ret;
+       
+       become_root();
+       check = pdb_getsampwrid(sam_user, q_u->rid);
+       unbecome_root();
+       
+       if (check != True)
+               return NT_STATUS_NO_SUCH_USER;
+       
+       uid = pdb_get_uid(sam_user);
+       if (uid == -1)
+               return NT_STATUS_NO_SUCH_USER;
+
+       pdb_free_sam(&sam_user);
+
+       if ((pwd=getpwuid(uid)) == NULL)
                return NT_STATUS_NO_SUCH_USER;
 
        if ((grp=getgrgid(map.gid)) == NULL)

diff --git a/source/utils/pdbedit.c b/source/utils/pdbedit.c
index 2ba6de55dfb48894da1c45abbc1c46bd6e695416..71abcc74eec680287c2e6b7d514a3bdbc3737451 100644 (file)
--- a/source/utils/pdbedit.c
+++ b/source/utils/pdbedit.c
@@ -155,27 +155,28 @@ static int print_user_info (char *username, BOOL verbosity, BOOL smbpwdstyle)
 static int print_users_list (BOOL verbosity, BOOL smbpwdstyle)
 {
        SAM_ACCOUNT *sam_pwent=NULL;
-       BOOL ret;
+       BOOL check, ret;
        
-       pdb_init_sam(&sam_pwent);
        errno = 0; /* testing --simo */
-       ret = pdb_setsampwent(False);
-       if (ret && errno == ENOENT) {
+       check = pdb_setsampwent(False);
+       if (check && errno == ENOENT) {
                fprintf (stderr,"Password database not found!\n");
                exit(1);
        }
-       pdb_free_sam(&sam_pwent);
 
-       while ((NT_STATUS_IS_OK(pdb_init_sam(&sam_pwent)) 
-               && (ret = pdb_getsampwent (sam_pwent)))) {
+       check = True;
+       if (!(NT_STATUS_IS_OK(pdb_init_sam(&sam_pwent)))) return 1;
+
+       while (check && (ret = pdb_getsampwent (sam_pwent))) {
                if (verbosity)
                        printf ("---------------\n");
                print_sam_info (sam_pwent, verbosity, smbpwdstyle);
                pdb_free_sam(&sam_pwent);
+               check = NT_STATUS_IS_OK(pdb_init_sam(&sam_pwent));
        }
-       pdb_free_sam(&sam_pwent);
+       if (check) pdb_free_sam(&sam_pwent);
        
-       pdb_endsampwent ();
+       pdb_endsampwent();
        return 0;
 }