[Freeipa-devel] [PATCHES 134-136] extdom: handle ERANGE return code for getXXYYY_r()

[Martin Kosek]

On 03/09/2015 02:49 PM, Tomas Babej wrote:
> 
> On 03/06/2015 01:08 PM, Alexander Bokovoy wrote:
>> On Thu, 05 Mar 2015, Sumit Bose wrote:
>>> On Thu, Mar 05, 2015 at 09:16:36AM +0100, Sumit Bose wrote:
>>>> On Wed, Mar 04, 2015 at 06:14:53PM +0100, Sumit Bose wrote:
>>>> > On Wed, Mar 04, 2015 at 04:17:55PM +0200, Alexander Bokovoy wrote:
>>>> > > On Mon, 02 Mar 2015, Sumit Bose wrote:
>>>> > > >diff --git
>>>> a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>>>> b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>>>> > > >index
>>>> 20fdd62b20f28f5384cf83b8be5819f721c6c3db..84aeb28066f25f05a89d0c2d42e8b060e2399501
>>>> 100644
>>>> > > >--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>>>> > > >+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>>>> > > >@@ -49,6 +49,220 @@
>>>> > > >
>>>> > > >#define MAX(a,b) (((a)>(b))?(a):(b))
>>>> > > >#define SSSD_DOMAIN_SEPARATOR '@'
>>>> > > >+#define MAX_BUF (1024*1024*1024)
>>>> > > >+
>>>> > > >+
>>>> > > >+
>>>> > > >+static int get_buffer(size_t *_buf_len, char **_buf)
>>>> > > >+{
>>>> > > >+    long pw_max;
>>>> > > >+    long gr_max;
>>>> > > >+    size_t buf_len;
>>>> > > >+    char *buf;
>>>> > > >+
>>>> > > >+    pw_max = sysconf(_SC_GETPW_R_SIZE_MAX);
>>>> > > >+    gr_max = sysconf(_SC_GETGR_R_SIZE_MAX);
>>>> > > >+
>>>> > > >+    if (pw_max == -1 && gr_max == -1) {
>>>> > > >+        buf_len = 16384;
>>>> > > >+    } else {
>>>> > > >+        buf_len = MAX(pw_max, gr_max);
>>>> > > >+    }
>>>> > > Here you'd get buf_len equal to 1024 by default on Linux which is too
>>>> > > low for our use case. I think it would be beneficial to add one more
>>>> > > MAX(buf_len, 16384):
>>>> > > -    if (pw_max == -1 && gr_max == -1) {
>>>> > > -        buf_len = 16384;
>>>> > > -    } else {
>>>> > > -        buf_len = MAX(pw_max, gr_max);
>>>> > > -    }
>>>> > > +    buf_len = MAX(16384, MAX(pw_max, gr_max));
>>>> > >
>>>> > > with MAX(MAX(),..) you also get rid of if() statement as resulting
>>>> > > rvalue would be guaranteed to be positive.
>>>> >
>>>> > done
>>>> >
>>>> > >
>>>> > > The rest is going along the common lines but would it be better to
>>>> > > allocate memory once per LDAP client request rather than always ask for
>>>> > > it per each NSS call? You can guarantee a sequential use of the buffer
>>>> > > within the LDAP client request processing so there is no problem with
>>>> > > locks but having this memory re-allocated on subsequent
>>>> > > getpwnam()/getpwuid()/... calls within the same request processing seems
>>>> > > suboptimal to me.
>>>> >
>>>> > ok, makes sense, I moved get_buffer() back to the callers.
>>>> >
>>>> > New version attached.
>>>>
>>>> Please ignore this patch, I will send a revised version soon.
>>>
>>> Please find attached a revised version which properly reports missing
>>> objects and out-of-memory cases and makes sure buf and buf_len are in
>>> sync.
>> ACK to patches 0135 and 0136. This concludes the review, thanks!
>>
> 
> Pushed to master: c15a407cbfaed163a933ab137eed16387efe25d2

As per https://fedorahosted.org/freeipa/ticket/4908, we will need this in 4.1.x
also:

Pushed to ipa-4-1: ec7a55a05647c4abad4c2a1bb5b5094f1e1eec55

Martin