Bug 6814 - Fixes for problems reported by valgrind
Fixes for problems reported by valgrind
Status: RESOLVED FIXED
Product: Samba 3.4
Classification: Unclassified
Component: Winbind
3.4.2
x86 Linux
: P3 minor
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-15 05:10 UTC by Roel van Meer
Modified: 2010-03-02 06:27 UTC (History)
3 users (show)

See Also:


Attachments
Initialize event timeout (959 bytes, patch)
2009-10-15 05:45 UTC, Roel van Meer
no flags Details
Free password before feeding it to smb_xstrdup (362 bytes, patch)
2009-10-15 05:49 UTC, Roel van Meer
no flags Details
Free smbldap_state when freeing idmap_alloc_ldap (414 bytes, patch)
2009-10-15 06:26 UTC, Roel van Meer
no flags Details
Correct fix for "Free password before feeding it to smb_xstrdup" (1.79 KB, patch)
2009-10-15 18:52 UTC, Jeremy Allison
no flags Details
Valgrind output of winbindd -i after single connect with smbclient -L (130.63 KB, text/plain)
2009-10-16 03:17 UTC, Roel van Meer
no flags Details
smbldap_state is not freed when verify_idpool() fails (516 bytes, patch)
2009-10-26 11:44 UTC, Roel van Meer
no flags Details
level 11 winbind debug log of crash (5.45 KB, application/octet-stream)
2009-11-24 07:30 UTC, Roel van Meer
no flags Details
Initialize event timeout, 3.5.0rc3 (493 bytes, patch)
2010-02-26 06:58 UTC, Roel van Meer
no flags Details
git-am fix for 3.5.0 final. (1.19 KB, patch)
2010-02-26 16:57 UTC, Jeremy Allison
vl: review+
jra: review? (metze)
Details
git-am fix for 3.4.7. (1.89 KB, patch)
2010-02-26 17:12 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Roel van Meer 2009-10-15 05:10:13 UTC
While researching winbindd core dumps, I came across some minor things that valgrind warned about. Hopefully these (small) patches are useful.
Comment 1 Roel van Meer 2009-10-15 05:12:21 UTC
First problem: complaints about uninitialised value.
These occur in the event timeout handling.

==22053== Conditional jump or move depends on uninitialised value(s)
==22053==    at 0x11A0DB: timeval_min (in /usr/sbin/winbindd)
==22053==    by 0x152503: event_add_to_select_args (in /usr/sbin/winbindd)
==22053==    by 0xB5810: schedule_async_request (in /usr/sbin/winbindd)
==22053==    by 0x8A05D: init_child_connection (in /usr/sbin/winbindd)
==22053==    by 0xB6FD4: async_domain_request (in /usr/sbin/winbindd)
==22053==    by 0x8A2E2: add_trusted_domains (in /usr/sbin/winbindd)
==22053==    by 0x8A864: rescan_trusted_domains (in /usr/sbin/winbindd)
==22053==    by 0x7F2A8: main (in /usr/sbin/winbindd)

==22053== Conditional jump or move depends on uninitialised value(s)
==22053==    at 0x11A0F3: timeval_min (in /usr/sbin/winbindd)
==22053==    by 0x152503: event_add_to_select_args (in /usr/sbin/winbindd)
==22053==    by 0xB5810: schedule_async_request (in /usr/sbin/winbindd)
==22053==    by 0x8A05D: init_child_connection (in /usr/sbin/winbindd)
==22053==    by 0xB6FD4: async_domain_request (in /usr/sbin/winbindd)
==22053==    by 0x8A2E2: add_trusted_domains (in /usr/sbin/winbindd)
==22053==    by 0x8A864: rescan_trusted_domains (in /usr/sbin/winbindd)
==22053==    by 0x7F2A8: main (in /usr/sbin/winbindd)

==22068== Conditional jump or move depends on uninitialised value(s)
==22068==    at 0x11A0DB: timeval_min (in /usr/sbin/winbindd)
==22068==    by 0x152503: event_add_to_select_args (in /usr/sbin/winbindd)
==22068==    by 0xB5810: schedule_async_request (in /usr/sbin/winbindd)
==22068==    by 0xBA20F: do_async (in /usr/sbin/winbindd)
==22068==    by 0xBEE10: winbindd_sid2gid_async (in /usr/sbin/winbindd)
==22068==    by 0x9F74F: sid2gid_lookupsid_recv (in /usr/sbin/winbindd)
==22068==    by 0xB9C91: lookupsid_recv (in /usr/sbin/winbindd)
==22068==    by 0xB76EB: do_async_recv (in /usr/sbin/winbindd)
==22068==    by 0xB68E4: async_reply_recv (in /usr/sbin/winbindd)
==22068==    by 0x7DF4C: rw_callback (in /usr/sbin/winbindd)
==22068==    by 0x7F508: main (in /usr/sbin/winbindd)
Comment 2 Roel van Meer 2009-10-15 05:45:54 UTC
Created attachment 4851 [details]
Initialize event timeout
Comment 3 Roel van Meer 2009-10-15 05:49:49 UTC
Created attachment 4852 [details]
Free password before feeding it to smb_xstrdup


Patch fixes this warning:

==25936== 27 bytes in 1 blocks are possibly lost in loss record 77 of 338
==25936==    at 0x48237D8: malloc (vg_replace_malloc.c:195)
==25936==    by 0x4AB8B8F: strdup (in /lib/libc-2.7.so)
==25936==    by 0x225A1C: smb_xstrdup (in /usr/sbin/winbindd)
==25936==    by 0x2AA911: fetch_ldap_pw (in /usr/sbin/winbindd)
==25936==    by 0x53B60D: ??? (in /usr/sbin/winbindd)
==25936==    by 0x53C489: ??? (in /usr/sbin/winbindd)
==25936==    by 0x53CC3A: smbldap_search_suffix (in /usr/sbin/winbindd)
==25936==    by 0x53D844: smbldap_search_domain_info (in /usr/sbin/winbindd)
==25936==    by 0x1E69EA: pdb_init_ldapsam (in /usr/sbin/winbindd)
==25936==    by 0x1D3C58: make_pdb_method_name (in /usr/sbin/winbindd)
==25936==    by 0x1D40DD: ??? (in /usr/sbin/winbindd)
==25936==    by 0x1D43DA: pdb_sid_to_id (in /usr/sbin/winbindd)
Comment 4 Roel van Meer 2009-10-15 06:26:49 UTC
Created attachment 4853 [details]
Free smbldap_state when freeing idmap_alloc_ldap

When idmap_ldap_alloc_init() TALLOC_FREE's idmap_alloc_ldap, it should also free idmap_alloc_ldap->smbldap_state, shouldn't it?

I'm still unsure whether or not smbldap_free_struct() should be called even when NT_STATUS_IS_OK( ret ) is true here. Or does that happen from idmap_ldap_close_destructor()?
Comment 5 Volker Lendecke 2009-10-15 14:23:51 UTC
The first patch is obviously correct. I'm not 100% sure about the second and third ones.

The second one: Shouldn't we do the SAFE_FREE at the beginning of the if-block to also catch all the error returns?

The third one: I think it is not valid for the first "goto done;" error condition (smbldap_init failed) to do the free. I think that belongs into the get_credentials() failure branch only.

Comments?

Volker
Comment 6 Jeremy Allison 2009-10-15 17:32:46 UTC
The second patch is not correct. The actual issue is that the "bind_dn" and the "bind_secret" elements are not freed when smbldap_close() is called, and are allocated in fetch_ldap_pw().. I'll fix this for master and 3.5.0.
Jeremy.
Comment 7 Jeremy Allison 2009-10-15 18:52:47 UTC
Created attachment 4859 [details]
Correct fix for "Free password before feeding it to smb_xstrdup"

This is the fix I'm going to add to master and 3.5.x for the memory leak.

==25936== 27 bytes in 1 blocks are possibly lost in loss record 77 of 338
==25936==    at 0x48237D8: malloc (vg_replace_malloc.c:195)
==25936==    by 0x4AB8B8F: strdup (in /lib/libc-2.7.so)
==25936==    by 0x225A1C: smb_xstrdup (in /usr/sbin/winbindd)
==25936==    by 0x2AA911: fetch_ldap_pw (in /usr/sbin/winbindd)
==25936==    by 0x53B60D: ??? (in /usr/sbin/winbindd)
==25936==    by 0x53C489: ??? (in /usr/sbin/winbindd)
==25936==    by 0x53CC3A: smbldap_search_suffix (in /usr/sbin/winbindd)
==25936==    by 0x53D844: smbldap_search_domain_info (in /usr/sbin/winbindd)
==25936==    by 0x1E69EA: pdb_init_ldapsam (in /usr/sbin/winbindd)
==25936==    by 0x1D3C58: make_pdb_method_name (in /usr/sbin/winbindd)
==25936==    by 0x1D40DD: ??? (in /usr/sbin/winbindd)
==25936==    by 0x1D43DA: pdb_sid_to_id (in /usr/sbin/winbindd)

I'd appreciate it if you also test it.
Thanks !
Jeremy.
Comment 8 Roel van Meer 2009-10-16 03:11:39 UTC
(In reply to comment #7)

> Correct fix for "Free password before feeding it to smb_xstrdup"
> I'd appreciate it if you also test it.

I've tested it with 3.4.2: the leak seems gone.
There still are some others though; I'll see if I can provide some more fixes/hints, if you're interested.

Regards,
roel
Comment 9 Roel van Meer 2009-10-16 03:17:43 UTC
Created attachment 4861 [details]
Valgrind output of winbindd -i after single connect with smbclient -L 

Valgrind output produced by running winbindd -i under valgrind and connecting to the samba server once, with "smbclient -L localhost -U user%pass".
Comment 10 Jeremy Allison 2009-10-16 14:47:52 UTC
Thanks for testing. I'll take a look at these. I'm guessing it's not shutting down correctly from the look of things.
Jeremy.
Comment 11 Jeremy Allison 2009-10-16 19:39:38 UTC
Ok, I'm extremely confused by these valgrind errors:

==22205== 11 bytes in 1 blocks are possibly lost in loss record 39 of 339
==22205==    at 0x48237D8: malloc (vg_replace_malloc.c:195)
==22205==    by 0x4AB8B8F: strdup (in /lib/libc-2.7.so)
==22205==    by 0x539F13: smbldap_set_creds (smbldap.c:1855)
==22205==    by 0x55F6FC: get_credentials (idmap_ldap.c:142)
==22205==    by 0x55FEC8: idmap_ldap_db_init (idmap_ldap.c:879)
==22205==    by 0x559E9F: idmap_init_domain (idmap.c:318)
==22205==    by 0x55A789: idmap_find_domain (idmap.c:361)
==22205==    by 0x55BC06: idmap_new_mapping (idmap.c:668)
==22205==    by 0x55C302: idmap_sid_to_gid (idmap_util.c:289)
==22205==    by 0x1C6C45: winbindd_dual_sid2gid (winbindd_idmap.c:374)
==22205==    by 0x1BDD89: schedule_async_request (winbindd_dual.c:453)
==22205==    by 0x1C221F: do_async (winbindd_async.c:83)

which seem to be when the secret is being copied inside smbldap_set_creds(). After my patch smbldap_set_creds() should look like this:

1840 bool smbldap_set_creds(struct smbldap_state *ldap_state, bool anon, const char *dn, const char *secret)
1841 {
1842         ldap_state->anonymous = anon;
1843 
1844         /* free any previously set credential */
1845 
1846         SAFE_FREE(ldap_state->bind_dn);
1847         if (ldap_state->bind_secret) {
1848                 /* make sure secrets are zeroed out of memory */
1849                 memset(ldap_state->bind_secret, '\0', strlen(ldap_state->bind_secret));
1850                 SAFE_FREE(ldap_state->bind_secret);
1851         }
1852 
1853         if ( ! anon) {
1854                 ldap_state->bind_dn = SMB_STRDUP(dn);
1855                 ldap_state->bind_secret = SMB_STRDUP(secret);
1856         }
1857 
1858         return True;
1859 }

When ldap_state->bind_secret is assigned to in line 1855 IT SHOULD HAVE BEEN FREED at line 1850 (which is only triggered if ldap_state->bind_secret contains a non-NULL value).

I can't see how these memory losses are occurring. Can you check if your smbldap_set_creds() looks identical to this one please ?

Jeremy.
Comment 12 Roel van Meer 2009-10-19 03:50:59 UTC
(In reply to comment #11)

> I can't see how these memory losses are occurring. Can you check if your
> smbldap_set_creds() looks identical to this one please ?

I'm on a week's holiday now; I'll get back to this next week.

Thanks,

roel
Comment 13 Roel van Meer 2009-10-26 11:36:40 UTC
(In reply to comment #11)

> I can't see how these memory losses are occurring. Can you check if your
> smbldap_set_creds() looks identical to this one please ?

Yes, it does. Maybe valgrind is wrong here? (It does say "possibly lost" ..)

I'm still looking if I can provide you with a proper testcase with debugging, but so far I haven't found it.
Comment 14 Roel van Meer 2009-10-26 11:44:49 UTC
Created attachment 4888 [details]
smbldap_state is not freed when verify_idpool() fails
Comment 15 Roel van Meer 2009-10-26 11:47:06 UTC
(In reply to comment #5)

> The third one: I think it is not valid for the first "goto done;" error
> condition (smbldap_init failed) to do the free. I think that belongs into the
> get_credentials() failure branch only.

You are right. I posted a new patch which calls smbldap_free_struct() only when verify_idpool() fails.

Regards,

roel
Comment 16 Michael Adam 2009-11-20 18:24:47 UTC
reassigning to Jeremy, since he was looking at this bug...
Comment 17 Roel van Meer 2009-11-24 07:27:59 UTC
I have some more information about what happens here, related to the original crashes I have been trying to debug.
The following is what I believe is happening:

A client does an idmap-related request to winbindd (for example: wbinfo --allocate-gid)
Because I haven't configured an idmap, the idmap initialization fails. That causes idmap_ldap_alloc_init() to free idmap_alloc_ldap. However, there still is an event for smbldap_idle_fn(). When this event is run, I get either a crash or an infinite loop, continually calling run_events() with an event for smbldap_idle_fn().
Comment 18 Roel van Meer 2009-11-24 07:30:15 UTC
Created attachment 4993 [details]
level 11 winbind debug log of crash

This is a part of the debug log of winbindd, at the time of the crash. There are some additional debug statements in there to help clarify things for myself, I hope they're not confusing for you.
Comment 19 Guenther Deschner 2009-11-24 07:39:55 UTC
(In reply to comment #17)
> I have some more information about what happens here, related to the original
> crashes I have been trying to debug.
> The following is what I believe is happening:
> 
> A client does an idmap-related request to winbindd (for example: wbinfo
> --allocate-gid)
> Because I haven't configured an idmap, the idmap initialization fails. That
> causes idmap_ldap_alloc_init() to free idmap_alloc_ldap. However, there still
> is an event for smbldap_idle_fn(). When this event is run, I get either a crash
> or an infinite loop, continually calling run_events() with an event for
> smbldap_idle_fn().

infinite loops in smbldap_idle_fn sound familiar, see bug #6894

> 

Comment 20 Roel van Meer 2010-02-26 06:58:06 UTC
Created attachment 5424 [details]
Initialize event timeout, 3.5.0rc3

3.5.0 still has the uninitialized timer.
Comment 21 Roel van Meer 2010-02-26 10:35:58 UTC
(In reply to comment #11)

> Ok, I'm extremely confused by these valgrind errors:
> [snip]
> which seem to be when the secret is being copied inside smbldap_set_creds().

I think valgrind gives this error, not because ldap_state->bind_dn and bind_secret are assigned to while still in use, but because ldap_state->bind_dn and bind_secret are not free'd when ldap_state becomes unaddressable.

With 3.5.0rc3 I can still reproduce it, not identically, but with a similar code path:

==11147== 77 bytes in 3 blocks are definitely lost in loss record 7 of 32
==11147==    at 0x482281D: malloc (vg_replace_malloc.c:207)
==11147==    by 0x4AB4B8F: strdup (in /lib/libc-2.7.so)
==11147==    by 0x510D95: smbldap_set_creds (smbldap.c:1947)
==11147==    by 0x5129FD: another_ldap_try (smbldap.c:1110)
==11147==    by 0x513699: smbldap_search_ext (smbldap.c:1406)
==11147==    by 0x513E4A: smbldap_search_suffix (smbldap.c:1726)
==11147==    by 0x514CF5: smbldap_search_domain_info (smbldap_util.c:280)
==11147==    by 0x11A492: pdb_init_ldapsam (pdb_ldap.c:6617)
==11147==    by 0x106EE8: make_pdb_method_name (pdb_interface.c:156)
==11147==    by 0x10733D: pdb_get_methods_reload (pdb_interface.c:193)
==11147==    by 0x1073EE: pdb_enum_trusteddoms (pdb_interface.c:1931)
==11147==    by 0xC9565: sam_trusted_domains (winbindd_passdb.c:653)

With 3.5.0rc3 I can trigger this warning by running winbind under valgrind and at some point signaling it with ctrl-C. In that case, pdb_init_ldapsam, which has allocated ldap_state, returns without having free'd ldap_state->bind_dn and bind_secret. During normal operation I haven't seen it so far, so it might be just something that occurs when shutting down - e.g. not something to worry about.

Since Jeremy's patch is already included, the only possibly useful thing in this bug report is the tiny event-timeout patch.

Regards,

roel
Comment 22 Jeremy Allison 2010-02-26 16:57:49 UTC
Created attachment 5429 [details]
git-am fix for 3.5.0 final.

I think this needs to be in 3.5.0 final. Volker & metze please review.

Jeremy.
Comment 23 Jeremy Allison 2010-02-26 17:12:06 UTC
Created attachment 5430 [details]
git-am fix for 3.4.7.

Similar patch for 3.4.7.

Jeremy.
Comment 24 Volker Lendecke 2010-02-27 10:21:44 UTC
Karo, please pull into the release branches. Zero risk from my point of view, your call if it goes into 3.5.0 or 3.5.1.

Thanks,

Volker
Comment 25 Karolin Seeger 2010-03-02 06:27:48 UTC
Pushed to v3-4-test and v3-5-test.
Closing out bug report.

Thanks!