Author: vlendec Date: 2007-02-27 17:21:21 +0000 (Tue, 27 Feb 2007) New Revision: 21563
WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=rev&root=samba&rev=21563 Log: Fix a memleak: We only need dispinfo structs for "our" and for the builtin domain. Without this patch we leaked a DISPINFO for the (NULL) domain per samr_connect*() call. Volker Modified: branches/SAMBA_3_0/source/rpc_server/srv_samr_nt.c branches/SAMBA_3_0_25/source/rpc_server/srv_samr_nt.c Changeset: Modified: branches/SAMBA_3_0/source/rpc_server/srv_samr_nt.c =================================================================== --- branches/SAMBA_3_0/source/rpc_server/srv_samr_nt.c 2007-02-27 17:17:16 UTC (rev 21562) +++ branches/SAMBA_3_0/source/rpc_server/srv_samr_nt.c 2007-02-27 17:21:21 UTC (rev 21563) @@ -46,8 +46,6 @@ #define DISP_INFO_CACHE_TIMEOUT 10 typedef struct disp_info { - struct disp_info *next, *prev; - TALLOC_CTX *mem_ctx; DOM_SID sid; /* identify which domain this is. */ BOOL builtin_domain; /* Quick flag to check if this is the builtin domain. */ struct pdb_search *users; /* querydispinfo 1 and 4 */ @@ -65,8 +63,6 @@ /* We keep a static list of these by SID as modern clients close down all resources between each request in a complete enumeration. */ -static DISP_INFO *disp_info_list; - struct samr_info { /* for use by the \PIPE\samr policy */ DOM_SID sid; @@ -254,49 +250,59 @@ Fetch or create a dispinfo struct. ********************************************************************/ -static DISP_INFO *get_samr_dispinfo_by_sid(DOM_SID *psid, const char *sid_str) +static DISP_INFO *get_samr_dispinfo_by_sid(DOM_SID *psid) { - TALLOC_CTX *mem_ctx; - DISP_INFO *dpi; + /* + * We do a static cache for DISP_INFO's here. Explanation can be found + * in Jeremy's checkin message to r11793: + * + * Fix the SAMR cache so it works across completely insane + * client behaviour (ie.: + * open pipe/open SAMR handle/enumerate 0 - 1024 + * close SAMR handle, close pipe. + * open pipe/open SAMR handle/enumerate 1024 - 2048... + * close SAMR handle, close pipe. + * And on ad-nausium. Amazing.... probably object-oriented + * client side programming in action yet again. + * This change should *massively* improve performance when + * enumerating users from an LDAP database. + * Jeremy. + * + * "Our" and the builtin domain are the only ones where we ever + * enumerate stuff, so just cache 2 entries. + */ + static struct disp_info builtin_dispinfo; + static struct disp_info domain_dispinfo; + /* There are two cases to consider here: 1) The SID is a domain SID and we look for an equality match, or 2) This is an account SID and so we return the DISP_INFO* for our domain */ - if ( psid && sid_check_is_in_our_domain( psid ) ) { - DEBUG(10,("get_samr_dispinfo_by_sid: Replacing %s with our domain SID\n", - sid_str)); - psid = get_global_sam_sid(); + if (psid == NULL) { + return NULL; } - for (dpi = disp_info_list; dpi; dpi = dpi->next) { - if (sid_equal(psid, &dpi->sid)) { - return dpi; - } + if (sid_check_is_builtin(psid) || sid_check_is_in_builtin(psid)) { + /* + * Necessary only once, but it does not really hurt. + */ + sid_copy(&builtin_dispinfo.sid, &global_sid_Builtin); + + return &builtin_dispinfo; } + + if (sid_check_is_domain(psid) || sid_check_is_in_our_domain(psid)) { + /* + * Necessary only once, but it does not really hurt. + */ + sid_copy(&domain_dispinfo.sid, get_global_sam_sid()); - /* This struct is never free'd - I'm using talloc so we - can get a list out of smbd using smbcontrol. There will - be one of these per SID we're authorative for. JRA. */ - - mem_ctx = talloc_init("DISP_INFO for domain sid %s", sid_str); - - if ((dpi = TALLOC_ZERO_P(mem_ctx, DISP_INFO)) == NULL) - return NULL; - - dpi->mem_ctx = mem_ctx; - - if (psid) { - sid_copy( &dpi->sid, psid); - dpi->builtin_domain = sid_check_is_builtin(psid); - } else { - dpi->builtin_domain = False; + return &domain_dispinfo; } - DLIST_ADD(disp_info_list, dpi); - - return dpi; + return NULL; } /******************************************************************* @@ -330,13 +336,8 @@ } info->mem_ctx = mem_ctx; - info->disp_info = get_samr_dispinfo_by_sid(psid, sid_str); + info->disp_info = get_samr_dispinfo_by_sid(psid); - if (!info->disp_info) { - talloc_destroy(mem_ctx); - return NULL; - } - return info; } Modified: branches/SAMBA_3_0_25/source/rpc_server/srv_samr_nt.c =================================================================== --- branches/SAMBA_3_0_25/source/rpc_server/srv_samr_nt.c 2007-02-27 17:17:16 UTC (rev 21562) +++ branches/SAMBA_3_0_25/source/rpc_server/srv_samr_nt.c 2007-02-27 17:21:21 UTC (rev 21563) @@ -46,8 +46,6 @@ #define DISP_INFO_CACHE_TIMEOUT 10 typedef struct disp_info { - struct disp_info *next, *prev; - TALLOC_CTX *mem_ctx; DOM_SID sid; /* identify which domain this is. */ BOOL builtin_domain; /* Quick flag to check if this is the builtin domain. */ struct pdb_search *users; /* querydispinfo 1 and 4 */ @@ -65,8 +63,6 @@ /* We keep a static list of these by SID as modern clients close down all resources between each request in a complete enumeration. */ -static DISP_INFO *disp_info_list; - struct samr_info { /* for use by the \PIPE\samr policy */ DOM_SID sid; @@ -254,49 +250,59 @@ Fetch or create a dispinfo struct. ********************************************************************/ -static DISP_INFO *get_samr_dispinfo_by_sid(DOM_SID *psid, const char *sid_str) +static DISP_INFO *get_samr_dispinfo_by_sid(DOM_SID *psid) { - TALLOC_CTX *mem_ctx; - DISP_INFO *dpi; + /* + * We do a static cache for DISP_INFO's here. Explanation can be found + * in Jeremy's checkin message to r11793: + * + * Fix the SAMR cache so it works across completely insane + * client behaviour (ie.: + * open pipe/open SAMR handle/enumerate 0 - 1024 + * close SAMR handle, close pipe. + * open pipe/open SAMR handle/enumerate 1024 - 2048... + * close SAMR handle, close pipe. + * And on ad-nausium. Amazing.... probably object-oriented + * client side programming in action yet again. + * This change should *massively* improve performance when + * enumerating users from an LDAP database. + * Jeremy. + * + * "Our" and the builtin domain are the only ones where we ever + * enumerate stuff, so just cache 2 entries. + */ + static struct disp_info builtin_dispinfo; + static struct disp_info domain_dispinfo; + /* There are two cases to consider here: 1) The SID is a domain SID and we look for an equality match, or 2) This is an account SID and so we return the DISP_INFO* for our domain */ - if ( psid && sid_check_is_in_our_domain( psid ) ) { - DEBUG(10,("get_samr_dispinfo_by_sid: Replacing %s with our domain SID\n", - sid_str)); - psid = get_global_sam_sid(); + if (psid == NULL) { + return NULL; } - for (dpi = disp_info_list; dpi; dpi = dpi->next) { - if (sid_equal(psid, &dpi->sid)) { - return dpi; - } + if (sid_check_is_builtin(psid) || sid_check_is_in_builtin(psid)) { + /* + * Necessary only once, but it does not really hurt. + */ + sid_copy(&builtin_dispinfo.sid, &global_sid_Builtin); + + return &builtin_dispinfo; } + + if (sid_check_is_domain(psid) || sid_check_is_in_our_domain(psid)) { + /* + * Necessary only once, but it does not really hurt. + */ + sid_copy(&domain_dispinfo.sid, get_global_sam_sid()); - /* This struct is never free'd - I'm using talloc so we - can get a list out of smbd using smbcontrol. There will - be one of these per SID we're authorative for. JRA. */ - - mem_ctx = talloc_init("DISP_INFO for domain sid %s", sid_str); - - if ((dpi = TALLOC_ZERO_P(mem_ctx, DISP_INFO)) == NULL) - return NULL; - - dpi->mem_ctx = mem_ctx; - - if (psid) { - sid_copy( &dpi->sid, psid); - dpi->builtin_domain = sid_check_is_builtin(psid); - } else { - dpi->builtin_domain = False; + return &domain_dispinfo; } - DLIST_ADD(disp_info_list, dpi); - - return dpi; + return NULL; } /******************************************************************* @@ -330,13 +336,8 @@ } info->mem_ctx = mem_ctx; - info->disp_info = get_samr_dispinfo_by_sid(psid, sid_str); + info->disp_info = get_samr_dispinfo_by_sid(psid); - if (!info->disp_info) { - talloc_destroy(mem_ctx); - return NULL; - } - return info; }
