svn commit: samba r9815 - in branches/SOC/SAMBA_4_0/source:
libnet torture/rpc
Stefan (metze) Metzmacher
metze at samba.org
Wed Aug 31 08:18:49 GMT 2005
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
brad at samba.org schrieb:
> WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=rev&root=samba&rev=9815
>
> Log:
> Added cldap_netlogon() AD Site-Name lookup into libnet/libnet_join.c.
> Bugfixing rampage in libnet_join.c to resolve misunderstanding of talloc_steal().
> libnet_join now creates the CN=<netbios name>,CN=Servers,CN=<site name>,CN=Sites,CN=Configuration,<domain dn> container on a dc join.
Hi Brad,
a few comments:
1) you should splitt patches up into logical pieces!
so this should have been 3 seperate commits
2) I think there're still some problems and bugs in this patch
Index: branches/SOC/SAMBA_4_0/source/libnet/config.mk
===================================================================
- --- branches/SOC/SAMBA_4_0/source/libnet/config.mk (revision 9814)
+++ branches/SOC/SAMBA_4_0/source/libnet/config.mk (revision 9815)
@@ -17,6 +17,8 @@
libnet/userinfo.o \
libnet/userman.o \
libnet/domain.o
- -REQUIRED_SUBSYSTEMS = RPC_NDR_SAMR RPC_NDR_LSA RPC_NDR_SRVSVC RPC_NDR_DRSUAPI LIBCLI_COMPOSITE
LIBCLI_RESOLVE LIBSAMBA3
+REQUIRED_SUBSYSTEMS = \
+ RPC_NDR_SAMR RPC_NDR_LSA RPC_NDR_SRVSVC RPC_NDR_DRSUAPI LIBCLI_COMPOSITE \
+ LIBCLI_RESOLVE LIBSAMBA3 LIBCLI_CLDAP NDR_ALL
why do we need NDR_ALL here?
Index: branches/SOC/SAMBA_4_0/source/libnet/libnet_join.c
===================================================================
- --- branches/SOC/SAMBA_4_0/source/libnet/libnet_join.c (revision 9814)
+++ branches/SOC/SAMBA_4_0/source/libnet/libnet_join.c (revision 9815)
@@ -130,13 +128,13 @@
- - lsa_open_policy.in.system_name = talloc_asprintf(tmp_ctx, "\\%s", r->in.netbios_name);
+ lsa_open_policy.in.system_name = talloc_asprintf(tmp_ctx, "\\%s", r->in.netbios_name);
the return value of talloc should be checked
@@ -148,12 +146,12 @@
- - status = dcerpc_lsa_QueryInfoPolicy2(c.out.dcerpc_pipe, tmp_ctx,
+ status = dcerpc_lsa_QueryInfoPolicy2(c.out.dcerpc_pipe, mem_ctx, /*chg tmp_ctx to mem_ctx*/
&lsa_query_info2);
tmp_ctx should be used here!
@@ -167,11 +165,11 @@
- - status = dcerpc_lsa_QueryInfoPolicy(c.out.dcerpc_pipe, tmp_ctx,
+ status = dcerpc_lsa_QueryInfoPolicy(c.out.dcerpc_pipe, mem_ctx, /*chg tmp_ctx to mem_ctx*/
&lsa_query_info);
tmp_ctx should be used here too!
- - status = dcerpc_parse_binding(tmp_ctx, c.out.dcerpc_pipe->conn->binding_string, &samr_binding);
+ status = dcerpc_parse_binding(mem_ctx, c.out.dcerpc_pipe->conn->binding_string, &samr_binding);
/*chg tmp_ctx to mem_ctx*/
tmp_ctx should be used here too!
+ r->out.domain_sid = talloc_reference(tmp_ctx, domain_sid);
+ r->out.domain_name = talloc_reference(tmp_ctx, domain_name);
+ r->out.realm = talloc_reference(tmp_ctx, realm);
+ r->out.samr_pipe = talloc_reference(tmp_ctx, samr_pipe);
+ r->out.samr_binding = talloc_reference(tmp_ctx, samr_binding);
+ r->out.user_handle = &u_handle;
+ r->out.error_string = talloc_reference(tmp_ctx, r2.samr_handle.out.error_string);
using tmp_ctx here will result in a mem leak
as you just increase the reference count,
as this values are supposed to be passed to the caller
this should be talloc_reference(mem_ctx, ...);
this will create a 2nd reference to the values from the callers TALLOC_CTX
and they get free'ed when the tmp_ctx AND the mem_ctx are free'ed.
but also in this case you can also use talloc_steal(mem_ctx, ...)
as talloc_steal() can never fail with a alloc failure,
where talloc_reference can!
and as we know that we'll destroy the tmp_ctx shortly,
talloc_steal is better.
+ computer_dn = talloc_asprintf(ctx,
+ "%s",r_crack_names.out.ctr.ctr1->array[0].result_name);
here we should also use talloc_steal(), with the right TALLOC_CTX.
(I haven't looked close to it to find the right one out.)
and also we should move that statement after the DsCrackNames call,
and make shure we didn't dereference a NULL pointer or empty array,
in case the server sends bad data.
so we need to check if r_crack_names.out.ctr.ctr1 and r_crack_names.out.ctr.ctr1->array[0]
and r_crack_names.out.ctr.ctr1->array[0].result_name are valid.
(we may already do this checks, I haven't checked closer)
+ /* We do the dsbind in this function, so we might as well do the domain_dn cracknames here too. */
+ r_crack_names.in.req.req1.format_offered = DRSUAPI_DS_NAME_FORMAT_NT4_ACCOUNT;
+ r_crack_names.in.req.req1.format_desired = DRSUAPI_DS_NAME_FORMAT_FQDN_1779;
+ names[0].str = talloc_asprintf(ctx, "%s\\", r->in.domain_name);
this should be tmp_ctx not ctx.
+ status = dcerpc_drsuapi_DsCrackNames(drsuapi_pipe, tmp_ctx, &r_crack_names);
+ if (!NT_STATUS_IS_OK(status)) {
+ if (NT_STATUS_EQUAL(status, NT_STATUS_NET_WRITE_FAULT)) {
+ r->out.error_string
+ = talloc_asprintf(tmp_ctx,
here we need to use mem_ctx!!!
+ } else {
+ r->out.error_string
+ = talloc_asprintf(tmp_ctx,
here we need to use mem_ctx too!!!
+ } else if (!W_ERROR_IS_OK(r_crack_names.out.result)) {
+ r->out.error_string
+ = talloc_asprintf(tmp_ctx,
here we need to use mem_ctx too!!!
+ domain_dn = talloc_asprintf(ctx,
+ "%s",
+ r_crack_names.out.ctr.ctr1->array[0].result_name);
talloc_steal() here...
and maybe tmp_ctx!
+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)
- - 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
- - 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_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, ...)
- --
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-----
More information about the samba-cvs
mailing list