svn commit: samba r9815 - in branches/SOC/SAMBA_4_0/source: libnet torture/rpc

[Brad Henry]

Hi again Metze, I have a couple more questions from your comments below.

Stefan (metze) Metzmacher wrote:

>
>
>+NTSTATUS libnet_JoinSite(struct libnet_context *ctx,
>+				TALLOC_CTX *mem_ctx,
>+				struct dcerpc_pipe *drsuapi_pipe,
>+				struct policy_handle drsuapi_bind_handle,
>+				char *computer_dn,
>+				char *domain_dn,
>these should be const char *
>
>
>+	cldap = cldap_socket_init(tmp_ctx, NULL);
>+	status = cldap_netlogon(cldap, tmp_ctx, &search);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		/* Default to using Default-First-Site-Name rather than returning status at this point. */
>+		site_name = talloc_asprintf(tmp_ctx, "%s", "Default-First-Site-Name");
>+	} else {
>+		site_name = talloc_steal(tmp_ctx, search.out.netlogon.logon5.site_name);
>+	}
>talloc fail checks
>
>
>+	config_dn = talloc_asprintf(tmp_ctx, "CN=Configuration,%s", domain_dn);
>+		
>+	server_dn = talloc_asprintf(tmp_ctx,
>+				"CN=%s,CN=Servers,CN=%s,CN=Sites,%s",
>+				libnet_r->in.netbios_name, site_name, config_dn);
>+
>+	schema_dn = talloc_asprintf(tmp_ctx, "CN=Schema,%s", config_dn);
>+
>+	ntds_dn = talloc_asprintf(tmp_ctx,
>+				"CN=NTDS Settings,%s", server_dn);
>talloc fail checks...
>
>
>+	printf("domain_dn: %s.\n", domain_dn);
>+	printf("computer_dn: %s.\n", computer_dn);
>+	printf("config_dn: %s.\n", config_dn);
>+	printf("server_dn: %s.\n", server_dn);
>+	printf("schema_dn: %s.\n", schema_dn);
>+	printf("ntds_dn: %s.\n", ntds_dn);
>+	printf("site_name: %s.\n", site_name);
>use DEBUG() instead of printf here
>printf should only be used in torture tests
>
>
>+	/*
>+	 Add entry CN=<netbios name>,CN=Servers,CN=<site name>,CN=Sites,CN=Configuration,<domain dn>.
>+	*/	
>+	remote_ldb_url = talloc_asprintf(tmp_ctx, "ldap://%s",
>+					  libnet_r->out.samr_binding->host);
>talloc fails check
>
>
>+	msg = ldb_msg_new(tmp_ctx);
>+	if (!msg) {
>+		return NT_STATUS_NO_MEMORY;
>+	}
>+	ldb_msg_add_string(remote_ldb, msg, "objectClass", "server");
>+	ldb_msg_add_string(remote_ldb, msg, "systemFlags", "50000000");
>+	ldb_msg_add_string(remote_ldb, msg, "serverReference", computer_dn);
>ldb_msg_add_string can also fails,
>(I know other places also didn't check the return value...,
> but that's bad!)
>
>
> 	if ((r->in.netbios_name != NULL) && (r->in.level != LIBNET_JOIN_AUTOMATIC)) {
>- -		r2->in.netbios_name = r->in.netbios_name;
>+		r2.in.netbios_name = r->in.netbios_name;
> 	} else {
>- -		r2->in.netbios_name = talloc_asprintf(r2, "%s", lp_netbios_name());
>+		r2.in.netbios_name = talloc_asprintf(mem_ctx, "%s", lp_netbios_name());
>talloc checks, and tmp_ctx (maybe we can skip the talloc_asprintf completely here)
>
>
>  
>
I found that the call would segfault here if I used talloc_steal() or 
talloc_reference() instead of talloc_asprintf().
Also, there's no tmp_ctx in this function, do you think I should create one?

>- -	r2->in.account_name = talloc_asprintf(r2, "%s$", r2->in.netbios_name);
>+	r2.in.account_name = talloc_asprintf(mem_ctx, "%s$", r2.in.netbios_name);
>talloc checks, and tmp_ctx
>
>
>- -	r->out.samr_pipe = talloc_steal(r, r2->out.samr_pipe);
>- -	/*r->out.user_handle = talloc_steal(mem_ctx, r2.out.user_handle);*/
>- -	r->out.user_handle = talloc_steal(r, r2->out.user_handle);
>- -	r->out.domain_sid = talloc_steal(r, r2->out.domain_sid);
>- -	r->out.join_password = talloc_steal(r, r2->out.join_password);
>+	r->out.samr_pipe = r2.out.samr_pipe;
>+	r->out.user_handle = r2.out.user_handle;
>+	r->out.domain_sid = r2.out.domain_sid;
>+	r->out.join_password = r2.out.join_password;
>talloc_steal(mem_ctx, ..) here
>
>  
>
If I used talloc_steal(mem_ctx, ...) here rather than a straight 
assignment, I get a segfault. Do you think the likely cause of that 
would be an indication of a problem in libnet_JoinDomain()?

>- -	msg->dn = ldb_dn_build_child(mem_ctx, "flatname", r2->out.domain_name, base_dn);
>+	msg->dn = ldb_dn_build_child(mem_ctx, "flatname", r2.out.domain_name, base_dn);
>this can fail!
>
>
>- -	samdb_msg_add_string(ldb, mem_ctx, msg, "flatname", r2->out.domain_name);
>- -	if (r2->out.realm) {
>- -		samdb_msg_add_string(ldb, mem_ctx, msg, "realm", r2->out.realm);
>+	samdb_msg_add_string(ldb, mem_ctx, msg, "flatname", r2.out.domain_name);
>+	if (r2.out.realm) {
>+		samdb_msg_add_string(ldb, mem_ctx, msg, "realm", r2.out.realm);
> 	}
> 	samdb_msg_add_string(ldb, mem_ctx, msg, "objectClass", "primaryDomain");
>- -	samdb_msg_add_string(ldb, mem_ctx, msg, "secret", r2->out.join_password);
>+	samdb_msg_add_string(ldb, mem_ctx, msg, "secret", r2.out.join_password);
> 	
>- -	samdb_msg_add_string(ldb, mem_ctx, msg, "samAccountName", r2->in.account_name);
>+	samdb_msg_add_string(ldb, mem_ctx, msg, "samAccountName", r2.in.account_name);
> 	
> 	samdb_msg_add_string(ldb, mem_ctx, msg, "secureChannelType", sct);
>samdb_msg_add_string can fail, and use tmp_ctx
>
>
>- -	if (r2->out.kvno) {
>+	if (r2.out.kvno) {
> 		samdb_msg_add_uint(ldb, mem_ctx, msg, "msDS-KeyVersionNumber",
>- -				   r2->out.kvno);
>+				   r2.out.kvno);
>samdb_msg_add_uint can fail(I think but need to checked), and use tmp_ctx
>
>  
>
samdb_msg_add_uint() does indeed need to be checked, as it performs a 
talloc_asprintf() and samdb_msg_add_string() which also could fail.
I put all of the error tests you've suggested in my last commit (r9858).

>
>- -		samdb_msg_set_string(ldb, mem_ctx, msg, "secret", r2->out.join_password);
>+		samdb_msg_set_string(ldb, mem_ctx, msg, "secret", r2.out.join_password);
>samdb_msg_add_string can fail, and use tmp_ctx
>
>
>- -		samdb_msg_set_string(ldb, mem_ctx, msg, "samAccountName", r2->in.account_name);
>+		samdb_msg_set_string(ldb, mem_ctx, msg, "samAccountName", r2.in.account_name);
> 		samdb_msg_set_string(ldb, mem_ctx, msg, "secureChannelType", sct);
>samdb_msg_add_string can fail, and use tmp_ctx
>
>
>- -	r->out.error_string = talloc_asprintf(r, "%s", r2->out.error_string);
>- -	r->out.join_password = talloc_steal(r, r2->out.join_password);
>- -	r->out.domain_sid = talloc_steal(r, r2->out.domain_sid);
>- -	r->out.samr_pipe = talloc_steal(r, r2->out.samr_pipe);
>- -	r->out.user_handle = talloc_steal(r, r2->out.user_handle);
>- -	/*r->out.user_handle = talloc_steal(mem_ctx, r2->out.user_handle);*/
>+	/*
>+	r->out.error_string = talloc_steal(r, r2.out.error_string);
>+	r->out.join_password = talloc_steal(r, r2.out.join_password);
>+	r->out.domain_sid = talloc_steal(r, r2.out.domain_sid);
>+	r->out.samr_pipe = talloc_steal(r, r2.out.samr_pipe);
>+	r->out.user_handle = talloc_steal(r, r2.out.user_handle);
>+	*/
>please don't commit this kind of comments, just remove them:-)
>
>
>+	r->out.error_string = r2.out.error_string;
>+	r->out.join_password = r2.out.join_password;
>+	r->out.domain_sid = r2.out.domain_sid;
>+	r->out.samr_pipe = r2.out.samr_pipe;
>+	r->out.user_handle = r2.out.user_handle;
>talloc_steal(mem_ctx, ...)
>
>  
>
This part also segfaults if I talloc_steal() rather than use direct 
assignment.

>- --
>metze
>
>Stefan Metzmacher <metze at samba.org> www.samba.org
>-----BEGIN PGP SIGNATURE-----
>Version: GnuPG v1.2.3-nr1 (Windows XP)
>Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
>
>iD8DBQFDFWfnm70gjA5TCD8RAv9FAJ90i0wGgrpzvQBhron5ZN1HWseQ0wCgnzrp
>1y/9EyaIjs+Z6wIZqBEBhyU=
>=PDzh
>-----END PGP SIGNATURE-----
>  
>
Thanks again for all your help,
Brad