[PATCH] add missing talloc_free
Julia Lawall
julia.lawall at lip6.fr
Sat Feb 25 23:22:06 MST 2012
On Sat, 25 Feb 2012, Richard Sharpe wrote:
> On Sat, Feb 25, 2012 at 12:32 PM, Julia Lawall <julia.lawall at lip6.fr> wrote:
>> There seem to be a number of places in the samba code where calls to
>> talloc_free are omitted. This patch fixes some cases.
>
> Are you aware of the way that talloc works? It seems likely to me that
> when a talloc_free is done somewhere further up in the call stack that
> if frees all memory.
OK, I see from the behavior of talloc how this could be the case. But
still it seems odd to free a value on some failures and not free it on
others, unless there is a reason to do so.
thanks,
julia
>
> Of course, it is possible that you have found a problem.
>
>> These problems were found using Coccinelle (http://coccinelle.lip6.fr/). A
>> simplified version of the semantic match that finds this problem is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @s exists@
>> local idexpression r.e;
>> expression e1,e2;
>> position p1,p2;
>> statement S;
>> @@
>>
>> (
>> if (e == NULL || ...) S
>> |
>> if (<+...e = e2...+>) S
>> |
>> if (...) { ... when != talloc_free(e);
>> (
>> return <+...e...+>;
>> |
>> * return ...;
>> )
>> }
>> )
>> ... when != e = e1
>> when != &e
>> when any
>> talloc_free(e);
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <julia.lawall at lip6.fr>
>>
>> ---
>>
>> I am not very familiar with the samba code, so these could be completely
>> off. Feedback is most welcome.
>>
>> dfs_server/dfs_server_ad.c | 2 ++
>> libcli/cldap/cldap.c | 3 ++-
>> libcli/lsarpc/util_lsarpc.c | 3 ++-
>> source4/dns_server/dlz_bind9.c | 2 ++
>> source4/dns_server/dns_server.c | 1 +
>> source4/dsdb/common/util_samr.c | 7 +++++++
>> source4/libcli/dgram/netlogon.c | 1 +
>> source4/libcli/util/clilsa.c | 4 ++++
>> source4/libnet/libnet_samsync_ldb.c | 2 ++
>> source4/libnet/libnet_site.c | 1 +
>> source4/ntvfs/posix/pvfs_dirlist.c | 4 ++++
>> source4/ntvfs/posix/pvfs_resolve.c | 7 +++++--
>> source4/ntvfs/posix/pvfs_streams.c | 2 ++
>> source4/ntvfs/posix/pvfs_xattr.c | 1 +
>> source4/utils/oLschema2ldif.c | 3 ++-
>> source4/winbind/wb_samba3_protocol.c | 1 +
>> 16 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/dfs_server/dfs_server_ad.c b/dfs_server/dfs_server_ad.c
>> index 22d3263..0c46e54 100644
>> --- a/dfs_server/dfs_server_ad.c
>> +++ b/dfs_server/dfs_server_ad.c
>> @@ -201,6 +201,7 @@ static NTSTATUS get_dcs_insite(TALLOC_CTX *ctx, struct
>> ldb_context *ldb,
>>
>> dn = ldb_msg_find_attr_as_dn(ldb, ctx, r->msgs[i],
>> "serverReference");
>> if (!dn) {
>> + talloc_free(r);
>> return NT_STATUS_INTERNAL_ERROR;
>> }
>>
>> @@ -208,6 +209,7 @@ static NTSTATUS get_dcs_insite(TALLOC_CTX *ctx, struct
>> ldb_context *ldb,
>> if (ret != LDB_SUCCESS) {
>> DEBUG(2,(__location__ ": Search for computer on %s
>> failed - %s\n",
>> ldb_dn_get_linearized(dn),
>> ldb_errstring(ldb)));
>> + talloc_free(r);
>> return NT_STATUS_INTERNAL_ERROR;
>> }
>>
>> diff --git a/libcli/cldap/cldap.c b/libcli/cldap/cldap.c
>> index 3322bd8..aa383c5 100644
>> --- a/libcli/cldap/cldap.c
>> +++ b/libcli/cldap/cldap.c
>> @@ -357,7 +357,8 @@ NTSTATUS cldap_socket_init(TALLOC_CTX *mem_ctx,
>> * Here we know the address family of the remote address.
>> */
>> if (fam == NULL) {
>> - return NT_STATUS_INVALID_PARAMETER_MIX;
>> + status = NT_STATUS_INVALID_PARAMETER_MIX;
>> + goto nterror;
>> }
>>
>> ret = tsocket_address_inet_from_strings(c, fam,
>> diff --git a/libcli/lsarpc/util_lsarpc.c b/libcli/lsarpc/util_lsarpc.c
>> index 96c9848..4f40b95 100644
>> --- a/libcli/lsarpc/util_lsarpc.c
>> +++ b/libcli/lsarpc/util_lsarpc.c
>> @@ -311,7 +311,8 @@ static NTSTATUS
>> auth_info_2_trustauth_inout_blob(TALLOC_CTX *mem_ctx,
>> iopw,
>>
>> (ndr_push_flags_fn_t)ndr_push_trustAuthInOutBlob);
>> if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
>> - return NT_STATUS_INVALID_PARAMETER;
>> + status = NT_STATUS_INVALID_PARAMETER;
>> + goto done;
>> }
>>
>> status = NT_STATUS_OK;
>> diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c
>> index 34320e0..f97acdb 100644
>> --- a/source4/dns_server/dlz_bind9.c
>> +++ b/source4/dns_server/dlz_bind9.c
>> @@ -377,6 +377,7 @@ static isc_result_t b9_putrr(struct dlz_bind9_data
>> *state,
>> TALLOC_CTX *tmp_ctx = talloc_new(state);
>>
>> if (!b9_format(state, tmp_ctx, rec, &type, &data)) {
>> + talloc_free(tmp_ctx);
>> return ISC_R_FAILURE;
>> }
>>
>> @@ -417,6 +418,7 @@ static isc_result_t b9_putnamedrr(struct dlz_bind9_data
>> *state,
>> TALLOC_CTX *tmp_ctx = talloc_new(state);
>>
>> if (!b9_format(state, tmp_ctx, rec, &type, &data)) {
>> + talloc_free(tmp_ctx);
>> return ISC_R_FAILURE;
>> }
>>
>> diff --git a/source4/dns_server/dns_server.c
>> b/source4/dns_server/dns_server.c
>> index 25873c2..d1f0639 100644
>> --- a/source4/dns_server/dns_server.c
>> +++ b/source4/dns_server/dns_server.c
>> @@ -494,6 +494,7 @@ static NTSTATUS dns_add_socket(struct dns_server *dns,
>> &dns_socket->local_address);
>> if (ret != 0) {
>> status = map_nt_error_from_unix_common(errno);
>> + talloc_free(dns_socket);
>> return status;
>> }
>>
>> diff --git a/source4/dsdb/common/util_samr.c
>> b/source4/dsdb/common/util_samr.c
>> index 184dfd5..e8365b1 100644
>> --- a/source4/dsdb/common/util_samr.c
>> +++ b/source4/dsdb/common/util_samr.c
>> @@ -113,6 +113,7 @@ NTSTATUS dsdb_add_user(struct ldb_context *ldb,
>> } else if (acct_flags == ACB_WSTRUST) {
>> if (cn_name[cn_name_len - 1] != '$') {
>> ldb_transaction_cancel(ldb);
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_FOOBAR;
>> }
>> cn_name[cn_name_len - 1] = '\0';
>> @@ -122,6 +123,7 @@ NTSTATUS dsdb_add_user(struct ldb_context *ldb,
>> } else if (acct_flags == ACB_SVRTRUST) {
>> if (cn_name[cn_name_len - 1] != '$') {
>> ldb_transaction_cancel(ldb);
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_FOOBAR;
>> }
>> cn_name[cn_name_len - 1] = '\0';
>> @@ -296,11 +298,13 @@ NTSTATUS dsdb_add_domain_group(struct ldb_context
>> *ldb,
>>
>> "(&(sAMAccountName=%s)(objectclass=group))",
>> ldb_binary_encode_string(tmp_ctx,
>> groupname));
>> if (name != NULL) {
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_GROUP_EXISTS;
>> }
>>
>> msg = ldb_msg_new(tmp_ctx);
>> if (msg == NULL) {
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> @@ -368,6 +372,7 @@ NTSTATUS dsdb_add_domain_alias(struct ldb_context *ldb,
>>
>> if (ldb_transaction_start(ldb) != LDB_SUCCESS) {
>> DEBUG(0, ("Failed to start transaction in
>> dsdb_add_domain_alias(): %s\n", ldb_errstring(ldb)));
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_INTERNAL_ERROR;
>> }
>>
>> @@ -469,6 +474,7 @@ NTSTATUS dsdb_enum_group_mem(struct ldb_context *ldb,
>> if (ret != LDB_SUCCESS) {
>> DEBUG(1, ("dsdb_enum_group_mem: dsdb_search for %s failed:
>> %s\n",
>> ldb_dn_get_linearized(dn), ldb_errstring(ldb)));
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_INTERNAL_DB_CORRUPTION;
>> }
>>
>> @@ -482,6 +488,7 @@ NTSTATUS dsdb_enum_group_mem(struct ldb_context *ldb,
>>
>> members = talloc_array(mem_ctx, struct dom_sid,
>> member_el->num_values);
>> if (members == NULL) {
>> + talloc_free(tmp_ctx);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> diff --git a/source4/libcli/dgram/netlogon.c
>> b/source4/libcli/dgram/netlogon.c
>> index 0aa6864..0087707 100644
>> --- a/source4/libcli/dgram/netlogon.c
>> +++ b/source4/libcli/dgram/netlogon.c
>> @@ -74,6 +74,7 @@ NTSTATUS dgram_mailslot_netlogon_reply(struct
>> nbt_dgram_socket *dgmsock,
>>
>> status = push_nbt_netlogon_response(&blob, tmp_ctx, reply);
>> if (!NT_STATUS_IS_OK(status)) {
>> + talloc_free(tmp_ctx);
>> return status;
>> }
>>
>> diff --git a/source4/libcli/util/clilsa.c b/source4/libcli/util/clilsa.c
>> index 4a81457..05cc9b5 100644
>> --- a/source4/libcli/util/clilsa.c
>> +++ b/source4/libcli/util/clilsa.c
>> @@ -63,6 +63,7 @@ static NTSTATUS smblsa_connect(struct smbcli_state *cli)
>>
>> lsa->ipc_tree = smbcli_tree_init(cli->session, lsa, false);
>> if (lsa->ipc_tree == NULL) {
>> + talloc_free(lsa);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> @@ -219,11 +220,13 @@ NTSTATUS smblsa_lookup_sid(struct smbcli_state *cli,
>>
>> status = smblsa_connect(cli);
>> if (!NT_STATUS_IS_OK(status)) {
>> + talloc_free(mem_ctx2);
>> return status;
>> }
>>
>> sid = dom_sid_parse_talloc(mem_ctx2, sid_str);
>> if (sid == NULL) {
>> + talloc_free(mem_ctx2);
>> return NT_STATUS_INVALID_SID;
>> }
>>
>> @@ -286,6 +289,7 @@ NTSTATUS smblsa_lookup_name(struct smbcli_state *cli,
>>
>> status = smblsa_connect(cli);
>> if (!NT_STATUS_IS_OK(status)) {
>> + talloc_free(mem_ctx2);
>> return status;
>> }
>>
>> diff --git a/source4/libnet/libnet_samsync_ldb.c
>> b/source4/libnet/libnet_samsync_ldb.c
>> index 20f0046..2590175 100644
>> --- a/source4/libnet/libnet_samsync_ldb.c
>> +++ b/source4/libnet/libnet_samsync_ldb.c
>> @@ -1225,12 +1225,14 @@ NTSTATUS libnet_samsync_ldb(struct libnet_context
>> *ctx, TALLOC_CTX *mem_ctx, str
>> r->in.session_info,
>> 0);
>> if (!state->sam_ldb) {
>> + talloc_free(state);
>> return NT_STATUS_INTERNAL_DB_ERROR;
>> }
>>
>> state->pdb = privilege_connect(mem_ctx,
>> ctx->lp_ctx);
>> if (!state->pdb) {
>> + talloc_free(state);
>> return NT_STATUS_INTERNAL_DB_ERROR;
>> }
>>
>> diff --git a/source4/libnet/libnet_site.c b/source4/libnet/libnet_site.c
>> index 288e13d..a218c10 100644
>> --- a/source4/libnet/libnet_site.c
>> +++ b/source4/libnet/libnet_site.c
>> @@ -63,6 +63,7 @@ NTSTATUS libnet_FindSite(TALLOC_CTX *ctx, struct
>> libnet_context *lctx, struct li
>> r->in.cldap_port,
>> &dest_address);
>> if (ret != 0) {
>> + talloc_free(tmp_ctx);
>> r->out.error_string = NULL;
>> status = map_nt_error_from_unix_common(errno);
>> return status;
>> diff --git a/source4/ntvfs/posix/pvfs_dirlist.c
>> b/source4/ntvfs/posix/pvfs_dirlist.c
>> index 1bc91c1..8dfd7b3 100644
>> --- a/source4/ntvfs/posix/pvfs_dirlist.c
>> +++ b/source4/ntvfs/posix/pvfs_dirlist.c
>> @@ -120,6 +120,7 @@ NTSTATUS pvfs_list_start(struct pvfs_state *pvfs, struct
>> pvfs_filename *name,
>> if (!pattern) {
>> /* this should not happen, as pvfs_unix_path is supposed to
>> return an absolute path */
>> + talloc_free(dir);
>> return NT_STATUS_UNSUCCESSFUL;
>> }
>>
>> @@ -131,16 +132,19 @@ NTSTATUS pvfs_list_start(struct pvfs_state *pvfs,
>> struct pvfs_filename *name,
>>
>> dir->unix_path = talloc_strdup(dir, name->full_name);
>> if (!dir->unix_path) {
>> + talloc_free(dir);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> dir->pattern = talloc_strdup(dir, pattern);
>> if (dir->pattern == NULL) {
>> + talloc_free(dir);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> dir->dir = opendir(name->full_name);
>> if (!dir->dir) { + talloc_free(dir);
>> return pvfs_map_errno(pvfs, errno);
>> }
>>
>> diff --git a/source4/ntvfs/posix/pvfs_resolve.c
>> b/source4/ntvfs/posix/pvfs_resolve.c
>> index 837ea17..532d7f8 100644
>> --- a/source4/ntvfs/posix/pvfs_resolve.c
>> +++ b/source4/ntvfs/posix/pvfs_resolve.c
>> @@ -434,7 +434,7 @@ static NTSTATUS pvfs_reduce_name(TALLOC_CTX *mem_ctx,
>> }
>> if (err_count) {
>> if (flags & PVFS_RESOLVE_WILDCARD) err_count--;
>> -
>> + talloc_free(s);
>> if (err_count==1) {
>> return NT_STATUS_OBJECT_NAME_INVALID;
>> } else {
>> @@ -451,7 +451,10 @@ static NTSTATUS pvfs_reduce_name(TALLOC_CTX *mem_ctx,
>> continue;
>> }
>> if (ISDOTDOT(components[i])) {
>> - if (i < 1) return NT_STATUS_OBJECT_PATH_SYNTAX_BAD;
>> + if (i < 1) {
>> + talloc_free(s);
>> + return NT_STATUS_OBJECT_PATH_SYNTAX_BAD;
>> + }
>> memmove(&components[i-1], &components[i+1],
>> sizeof(char *)*(num_components-i));
>> i -= 2;
>> diff --git a/source4/ntvfs/posix/pvfs_streams.c
>> b/source4/ntvfs/posix/pvfs_streams.c
>> index cacd8c1..2507fe6 100644
>> --- a/source4/ntvfs/posix/pvfs_streams.c
>> +++ b/source4/ntvfs/posix/pvfs_streams.c
>> @@ -253,6 +253,7 @@ NTSTATUS pvfs_stream_rename(struct pvfs_state *pvfs,
>> struct pvfs_filename *name,
>>
>> new_name = stream_name_normalise(streams, new_name);
>> if (new_name == NULL) {
>> + talloc_free(streams);
>> return NT_STATUS_NO_MEMORY;
>> }
>>
>> @@ -264,6 +265,7 @@ NTSTATUS pvfs_stream_rename(struct pvfs_state *pvfs,
>> struct pvfs_filename *name,
>> /* the default stream always exists */
>> if (strcmp(new_name, "") == 0 ||
>> strcasecmp_m(new_name, ":$DATA") == 0) {
>> + talloc_free(streams);
>> return NT_STATUS_OBJECT_NAME_COLLISION;
>> }
>>
>> diff --git a/source4/ntvfs/posix/pvfs_xattr.c
>> b/source4/ntvfs/posix/pvfs_xattr.c
>> index 04ad178..59715cd 100644
>> --- a/source4/ntvfs/posix/pvfs_xattr.c
>> +++ b/source4/ntvfs/posix/pvfs_xattr.c
>> @@ -171,6 +171,7 @@ NTSTATUS pvfs_dosattrib_load(struct pvfs_state *pvfs,
>> struct pvfs_filename *name
>> }
>>
>> if (!(pvfs->flags & PVFS_FLAG_XATTR_ENABLE)) {
>> + talloc_free(mem_ctx);
>> return NT_STATUS_OK;
>> }
>>
>> diff --git a/source4/utils/oLschema2ldif.c b/source4/utils/oLschema2ldif.c
>> index ae69db1..4e39fb4 100644
>> --- a/source4/utils/oLschema2ldif.c
>> +++ b/source4/utils/oLschema2ldif.c
>> @@ -353,7 +353,8 @@ static struct ldb_message *process_entry(TALLOC_CTX
>> *mem_ctx, const char *entry)
>> ldb_msg_add_string(msg, "objectClass", "top");
>>
>> c = talloc_strdup(ctx, entry);
>> - if (!c) return NULL;
>> + if (!c)
>> + goto failed;
>>
>> c = skip_spaces(c);
>>
>> diff --git a/source4/winbind/wb_samba3_protocol.c
>> b/source4/winbind/wb_samba3_protocol.c
>> index 2846e9c..2899876 100644
>> --- a/source4/winbind/wb_samba3_protocol.c
>> +++ b/source4/winbind/wb_samba3_protocol.c
>> @@ -352,6 +352,7 @@ NTSTATUS wbsrv_samba3_process(struct wbsrv_samba3_call
>> *call)
>> status = wbsrv_samba3_pull_request(call);
>>
>> if (!NT_STATUS_IS_OK(status)) {
>> + talloc_free(call);
>> return status;
>> }
>>
>
>
>
> --
> Regards,
> Richard Sharpe
> (??????????--??)
>
More information about the samba-technical
mailing list