Bug 13836 - acl_read module accidentally turns searches for no attributes into searches for all attributes
Summary: acl_read module accidentally turns searches for no attributes into searches f...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.10.0rc4
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-12 22:19 UTC by Garming Sam
Modified: 2019-04-03 10:27 UTC (History)
3 users (show)

See Also:


Attachments
Patch for master (1.43 KB, patch)
2019-03-12 22:22 UTC, Garming Sam
no flags Details
improvement in timing (274.12 KB, image/png)
2019-03-31 22:11 UTC, Garming Sam
no flags Details
improvement in absolute timing (184.63 KB, image/png)
2019-03-31 22:12 UTC, Garming Sam
no flags Details
performance improvement json (85.54 KB, application/json)
2019-03-31 22:13 UTC, Garming Sam
no flags Details
Backported patch for 4.10 (7.58 KB, patch)
2019-04-01 00:29 UTC, Garming Sam
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Garming Sam 2019-03-12 22:19:19 UTC
This was a regression caused by db15fcfa899e1fe4d6994f68ceb299921b8aa6f1 for empty lists.
  
The original code never dereferenced attrs and only added "*" if attrs was NULL (not if attrs[0] was NULL).

This causes significant performance issues with the new paged_results module introduced for 4.10 as the initial GUID search requests no attributes. This GUID search turns into a search for "*" and ends up allocating memory for the entire database.

This never appears to cause changes in the final result set, only intermediate processing.
Comment 1 Garming Sam 2019-03-12 22:22:38 UTC
Created attachment 14927 [details]
Patch for master
Comment 2 Garming Sam 2019-03-12 22:32:51 UTC
After testing again, this actually did cause changes to the final result set.
Comment 3 Garming Sam 2019-03-12 22:46:37 UTC
Specifically, connecting via LDAP and connecting via the local database returns different answers. With this patch, the two now both return no attributes. We probably need some tests to make sure that this behaviour isn't accidentally broken again.
Comment 4 Garming Sam 2019-03-21 03:35:44 UTC
Right now triggering remote LDAP searches via LDB turns empty lists into searches for all attributes (as the LDAP specification dictates) because no mapping occurs.  

In order to align the answers from the local database and the remote we need to change the ildap module to translate empty lists into searches for the OID "1.1" as described in RFC 2251, Section 4.5.1: 'If the client does not want any attributes returned, it can specify a list containing only the attribute with OID "1.1".

For anyone who actually wants all the attributes, they can either use "*" or fail to supply an attribute list.
Comment 5 Garming Sam 2019-03-29 00:39:22 UTC
Bug filed separately for LDB discrepancies:

https://bugzilla.samba.org/show_bug.cgi?id=13852
Comment 6 Garming Sam 2019-03-31 22:11:41 UTC
Created attachment 15024 [details]
improvement in timing
Comment 7 Garming Sam 2019-03-31 22:12:04 UTC
Created attachment 15025 [details]
improvement in absolute timing
Comment 8 Garming Sam 2019-03-31 22:13:22 UTC
Created attachment 15026 [details]
performance improvement json
Comment 9 Garming Sam 2019-04-01 00:29:05 UTC
Created attachment 15027 [details]
Backported patch for 4.10
Comment 10 Andrew Bartlett 2019-04-01 00:57:51 UTC
Please select this performance regression fix for 4.10.

Thanks!
Comment 11 Karolin Seeger 2019-04-02 07:53:47 UTC
(In reply to Andrew Bartlett from comment #10)
Pushed to autobuild-v4-10-test.
Comment 12 Karolin Seeger 2019-04-03 10:27:19 UTC
(In reply to Karolin Seeger from comment #11)
Pushed to v4-10-test.
Closing out bug report.

Thanks!