Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RTREUSEWHERE in CREATEQUERY #1

Merged
merged 2 commits into from Mar 6, 2017

Conversation

lummax
Copy link

@lummax lummax commented Feb 20, 2017

First up: tremendous work!

While testing the WSP implementation with a Windows 8.1 I noticed an error in
the handling of the RTREUSEWHERE restriction. The code correctly identified
the case and called rtreusewhere_to_string(), but neither the share nor the
where_id where passed back, resulting in an error in run_new_query_send() as
the share was not set.

I believe with this, the find_query_info() in run_new_query_send() is
redundant, but I'm not quite sure.

I also added some more debug output to hunt that down.

Copy link
Owner

@noelpower noelpower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Lucas,
Sorry for the delay in getting back to you, just busy with something at the moment and hope to get some free time next week, I have in the mean time some questions.
Are you able to reproduce the bug? If so can you provide level 10 samba logs and a network trace for the attempt that fails ? I would really love to understand what happens there. I don't doubt that this happens, I have fixed some similar issue before, either I broke this again or this is a new variation (and I no longer recall how the previous variation manifested itself :-))

@lummax
Copy link
Author

lummax commented Feb 24, 2017

See log_and_capture.zip for log.wspd and network traces. The pre_patches* version is without commit a02623f but with added debug logging. The post_patches* is the fixed version.

I have rebased the WSP-WIP branch and my patches on 4.6.rc2 but that should not make a difference.

This is the interesting section with the new debug message run_new_query_send: No share passed in the RestrictionSet.

[2017/02/24 15:35:58.756276,  3, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_gss.c:1936(handle_wsp_send)
  handle_wsp_send: received CPMCREATEQUERY message from handle 1
[2017/02/24 15:35:58.756303,  5, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_gss.c:102(get_connected_client_entry)
  get_connected_client_entry: compare 0x1 with 0x2
[2017/02/24 15:35:58.756319,  5, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_gss.c:102(get_connected_client_entry)
  get_connected_client_entry: compare 0x1 with 0x1
[2017/02/24 15:35:58.756495, 10, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_sparql_conv.c:1555(rtreusewhere_to_string)
  rtreusewhere_to_string: SHARE reusewhereid 2
[2017/02/24 15:35:58.756515,  3, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_sparql_conv.c:1577(rtreusewhere_to_string)
  rtreusewhere_to_string: detected a where id RTREUSEWHERE id=2 result = (((false != true)) && tracker:uri-is-descendant ('file:///var/shares/test-share', nie:url (?u)))
[2017/02/24 15:35:58.756537,  0, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_srv_tracker_abs_if.c:379(run_new_query_send)
  run_new_query_send: No share passed in the RestrictionSet
[2017/02/24 15:35:58.756557, 10, pid=7422, effective(0, 0), real(0, 0), class=tevent] ../source3/lib/tevent_glib_glue.c:617(tevent_glib_process)
  tevent_glib_process: tevent_glib_process: num_ready: 0
[2017/02/24 15:35:58.756575, 10, pid=7422, effective(0, 0), real(0, 0), class=tevent] ../source3/lib/tevent_glib_glue.c:429(get_glib_fds_and_timeout)
  get_glib_fds_and_timeout: get_glib_fds_and_timeout: num fds: 1, timeout: -1 ms
[2017/02/24 15:35:58.756593,  0, pid=7422, effective(0, 0), real(0, 0)] ../source3/rpc_server/wsp/wsp_gss.c:1977(handle_wsp_done)
  handle_wsp_done: detected some async processing error NT_STATUS_INVALID_PARAMETER 0xc000000d for request 0x559ff9c76790

Thank you for taking the time to look at this :).

@noelpower
Copy link
Owner

Finally got a chance to look at the logs/trace. Yep, this is badly broken, this should (and used) to work, building the query using the whereid is a key requirement (I'd say you see more queries using a whereid than without on later windows clients) It looks I removed code this by accident at some point (hard to see because I rewrite the branch history to keep the number of patches for upstreaming manageable) This is the missing part I think your patch is neater so if you care to resubmit the patches with your signoff (please see https://wiki.samba.org/index.php/CodeReview#commit_message_tags) understanding that this is necessary for me to upstream this eventually. I don't want to be chasing sign-offs etc. later on so please read the wiki section especially paying attention to the copyright policy etc:-)

Signed-off-by: Lukas Oyen <oyen@univention.de>
@lummax lummax force-pushed the wsp-wip/fix-rtreuse-where branch from 842738d to a1b6ec1 Compare March 1, 2017 13:22
@lummax
Copy link
Author

lummax commented Mar 1, 2017

Copyright mail got send, patches are rebased to WSP-WIP and signed of. I deleted the code added in 0a9d82f as it's redundant now.

@noelpower
Copy link
Owner

Hi Lukas,
those changes look fine to me thanks! could you also incorporate removal of these lines in the commit, I think you are correct they are no longer necessary.

Copy link
Owner

@noelpower noelpower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Lukas, sorry for being a pain but could you possibly squash this commit with the "fix share-lookup on RTREUSEWHERE" one ? I think they really should be part of the one change

Signed-off-by: Lukas Oyen <oyen@univention.de>
@lummax lummax force-pushed the wsp-wip/fix-rtreuse-where branch from a5bf9d6 to 9eada8d Compare March 6, 2017 13:28
@lummax
Copy link
Author

lummax commented Mar 6, 2017

@noelpower no worries :)

Done!

@noelpower noelpower merged commit 15b8868 into noelpower:WSP-WIP Mar 6, 2017
@lummax lummax deleted the wsp-wip/fix-rtreuse-where branch March 7, 2017 13:07
noelpower pushed a commit that referenced this pull request Apr 5, 2018
The change for https://bugzilla.samba.org/show_bug.cgi?id=7587
("talloc_autofree_context() in shared libraries and plugins is a bad idea on FreeBSD")
(ommit 41b6810)
causes the following for sssd on Linux:

     Stack trace of thread 19667:
     #0  0x00007f2cab91ff6b __GI_raise (libc.so.6)
     #1  0x00007f2cab90a5c1 __GI_abort (libc.so.6)
     samba-team#2  0x00007f2cab90a491 __assert_fail_base (libc.so.6)
     samba-team#3  0x00007f2cab9186e2 __GI___assert_fail (libc.so.6)
     samba-team#4  0x00007f2cb10aaca5 k5_mutex_lock (libkrb5.so.3)
     samba-team#5  0x00007f2cb10ab790 k5_mutex_lock (libkrb5.so.3)
     samba-team#6  0x00007f2cb10ab8f5 profile_free_file (libkrb5.so.3)
     samba-team#7  0x00007f2cb10ab983 profile_close_file (libkrb5.so.3)
     samba-team#8  0x00007f2cb10af249 profile_release (libkrb5.so.3)
     samba-team#9  0x00007f2cb10a06c7 k5_os_free_context (libkrb5.so.3)
     samba-team#10 0x00007f2cb1075a9a krb5_free_context (libkrb5.so.3)
     samba-team#11 0x000055cea7cb2dd1 kcm_data_destructor (sssd_kcm)
     samba-team#12 0x00007f2cac153e96 _tc_free_internal (libtalloc.so.2)
     samba-team#13 0x00007f2cac1537b0 _tc_free_internal (libtalloc.so.2)
     samba-team#14 0x00007f2cac1537b0 _tc_free_internal (libtalloc.so.2)
     samba-team#15 0x00007f2cac1537b0 _tc_free_internal (libtalloc.so.2)
     samba-team#16 0x00007f2cac1537b0 _tc_free_internal (libtalloc.so.2)
     samba-team#17 0x00007f2cac14e648 _talloc_free (libtalloc.so.2)
     samba-team#18 0x00007f2cac14c480 talloc_lib_fini (libtalloc.so.2)
     samba-team#19 0x00007f2cb151da96 _dl_fini (ld-linux-x86-64.so.2)
     samba-team#20 0x00007f2cab9226bc __run_exit_handlers (libc.so.6)
     samba-team#21 0x00007f2cab9227ec __GI_exit (libc.so.6)
     samba-team#22 0x00007f2cb030dc61 orderly_shutdown (libsss_util.so)
     samba-team#23 0x00007f2cac365a46 tevent_common_check_signal (libtevent.so.0)
     samba-team#24 0x00007f2cac367975 epoll_event_loop_once (libtevent.so.0)
     samba-team#25 0x00007f2cac365dab std_event_loop_once (libtevent.so.0)
     samba-team#26 0x00007f2cac362098 _tevent_loop_once (libtevent.so.0)
     samba-team#27 0x00007f2cac3622eb tevent_common_loop_wait (libtevent.so.0)
     samba-team#28 0x00007f2cac365d3b std_event_loop_wait (libtevent.so.0)
     samba-team#29 0x00007f2cb030eb37 server_loop (libsss_util.so)
     samba-team#30 0x000055cea7cb29f4 main (sssd_kcm)
     samba-team#31 0x00007f2cab90c1eb __libc_start_main (libc.so.6)
     samba-team#32 0x000055cea7cb2c7a _start (sssd_kcm)

We still only register one atexit handler instead of multiple ones
like in talloc 2.1.11, but avoids using a library destructor.

Bug #7587 seems to be fixed by not using talloc_autofree_context()
within samba.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13366

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
noelpower pushed a commit that referenced this pull request Jul 4, 2019
Fixes Asan error:

==1924==ERROR: AddressSanitizer: stack-use-after-scope on address
    0x7ffe63f873d0 at pc 0x7fb99dae1733 bp 0x7ffe63f86a00 sp 0x7ffe63f861a8
READ of size 24 at 0x7ffe63f873d0 thread T0
    #0 0x7fb99dae1732  (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79732)
    #1 0x7fb99cfe5549 in memcpy
        /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34
    samba-team#2 0x7fb99cfe5549 in ndr_push_bytes
        ../../librpc/ndr/ndr_basic.c:729
    samba-team#3 0x7fb99cfe5646 in ndr_push_array_uint8
        ../../librpc/ndr/ndr_basic.c:754
    samba-team#4 0x7fb99a69dd1b in ndr_push_netr_ChallengeResponse
        librpc/gen_ndr/ndr_netlogon.c:462
    samba-team#5 0x7fb99a6c5fab in ndr_push_netr_NetworkInfo
        librpc/gen_ndr/ndr_netlogon.c:556
    samba-team#6 0x7fb99a6c749d in ndr_push_netr_LogonLevel
         librpc/gen_ndr/ndr_netlogon.c:783
    samba-team#7 0x7fb99a7222de in ndr_push_netr_LogonSamLogonEx
         librpc/gen_ndr/ndr_netlogon.c:16547
    samba-team#8 0x7fb99c982c97 in dcerpc_binding_handle_call_send
         ../../librpc/rpc/binding_handle.c:416

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13936

Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
Autobuild-Date(master): Fri May 10 10:02:21 UTC 2019 on sn-devel-184
noelpower pushed a commit that referenced this pull request Jul 4, 2019
Fix use after free detected by AddressSanitizer

AddressSanitizer: heap-use-after-free on address 0x60f0002b2738
                  at pc 0x7f89b1a213b5 bp 0x7ffce9528810 sp 0x7ffce9528800
                  READ of size 8 at 0x60f0002b2738 thread T0
    #0 0x7f89b1a213b4 in samldb_rename_search_base_callback
        ../../source4/dsdb/samdb/ldb_modules/samldb.c:4203
    #1 0x7f89d3a0db4a in ldb_module_send_entry
        ../../lib/ldb/common/ldb_modules.c:793
    samba-team#2 0x7f89b6f27356 in es_callback
        ../../source4/dsdb/samdb/ldb_modules/encrypted_secrets.c:1418

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13942

Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
noelpower pushed a commit that referenced this pull request Jul 4, 2019
Fix use after free detected by AddressSanitizer

AddressSanitizer: heap-use-after-free on address 0x61400026a4a0
                  at pc 0x7fd555c52f12 bp 0x7ffed7231180 sp 0x7ffed7231170
                  READ of size 1 at 0x61400026a4a0 thread T0
    #0 0x7fd555c52f11 in ldb_should_b64_encode
       ../../lib/ldb/common/ldb_ldif.c:197
    #1 0x7fd539dc9417 in dsdb_audit_add_ldb_value
       ../../source4/dsdb/samdb/ldb_modules/audit_util.c:491
    samba-team#2 0x7fd539dc9417 in dsdb_audit_attributes_json
       ../../source4/dsdb/samdb/ldb_modules/audit_util.c:651
    samba-team#3 0x7fd539dc6a7e in operation_json
       ../../source4/dsdb/samdb/ldb_modules/audit_log.c:305

The problem is that at the successful end of these functions
el->values is overwritten with new_values.  However get_parsed_dns()
points p->v at the supplied el and it effectively gets used
as a working area by replmd_build_la_val().  So we must duplicate it
because our caller only called ldb_msg_copy_shallow().

The reason this matters is that the audit_log module is
above repl_meta_data in the stack, and tries to log the
ldb_message it saw after the reply (to include the error code).
If that ldb_message is changed it is not only misleading,
it can point to memory that has since gone away.

In this case the memory for the full extended DN in the
member attribute ended up on 'ac', a context lost by
the time repl_meta_data has finished processing.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13941

Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>

Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
Autobuild-Date(master): Wed May 15 05:35:47 UTC 2019 on sn-devel-184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants