119380 - nsILDAPMessage needs to support binary attribute values

Status: VERIFIED FIXED

(Directory :: LDAP XPCOM SDK, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This will probably be necessary for S/MIME support.  We'll need to either modify
the ::getValues method signature, or else add another method.

Comment 1

[Kai Engert (:KaiE:)]

cc'ing ssaux

Comment 2

[Stephane Saux]

Dan, when can we get this?

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Not-yet-functional patch adding a getBinaryValues method, v1

This isn't yet working, but it's got the structure in place, and shows what the
interfaces will probably look like.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: hopefully functional patch adding a getBinaryValues method, v2

Peter, Kai: see what you think of this.  I've filled in the implementation part
of this, and it all even compiles.  Give it a shot and see if it fits your
needs...

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Note to self: still need to file a bug about merging with or obsoleting
nsIByteBuffer.h before I check this in.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 69198 [details] [diff] [review]
hopefully functional patch adding a getBinaryValues method, v2

>+nsLDAPBERValue::Get(PRUint32 *aCount, PRUint8 **aRetVal)
>+{
>+    if (!mValue) {
>+        return NS_ERROR_NOT_INITIALIZED;
>+    }
>+
>+    // get a buffer to hold a copy of the data
>+    //
>+    PRUint8 *array;
>+    array = NS_STATIC_CAST(PRUint8 *, nsMemory::Alloc(mSize));
>+    if (!array) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    
>+    // copy and return
>+    //
>+    memcpy(array, mValue, mSize);
>+    return NS_OK;
>+}
I think you want to set *aCount and *aRetVal here.

>+// void set(in unsigned long aCount, 
>+//          [array, size_is(aCount)] in octet aValue);
>+NS_IMETHODIMP
>+nsLDAPBERValue::Set(PRUint32 aCount, PRUint8 *aValue)
>+{
>+    // get rid of any old value being held here
>+    //
>+    if (mValue) {
>+        nsMemory::Free(mValue);
>+    }
>+
>+    // get a buffer to hold a copy of this data
>+    //
>+    mValue = NS_STATIC_CAST(PRUint8 *, nsMemory::Alloc(aCount));
>+    if (!mValue) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    // copy the data and return
>+    //
>+    memcpy(mValue, aValue, mSize);
>+    return NS_OK;
>+}
I think you want to set mSize to aCount.

>+// void setFromString(in AString aValue);
>+//
>+NS_IMETHODIMP
>+nsLDAPBERValue::SetFromString(const nsAString & aValue)
>+{
>+    // get rid of any old value being held here
>+    //
>+    if (mValue) {
>+        nsMemory::Free(mValue);
>+    }
>+
>+    // get a UTF8 version
>+    //
>+    NS_ConvertUCS2toUTF8 utf8Value(aValue);
>+
>+    // get a buffer to hold a copy of this data
>+    //
>+    // Note: at the time of this writing, .Length() appears to be the
>+    // only way to get the size in bytes of a particular
>+    // NS_ConvertUCS2toUTF8 string.  It wouldn't be entirely suprising
>+    // if the semantics of .Length were corrected, and .Size() were used
>+    // for that function, at which point this code would need to be tweaked
>+    // also.
>+    //
>+    mValue = NS_STATIC_CAST(PRUint8 *, nsMemory::Alloc(utf8Value.Length()));
>+    if (!mValue) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    // copy the data and return
>+    //
>+    memcpy(mValue, utf8Value.get(), utf8Value.Length());
>+    return NS_OK;
>+}
Same here.

>+    // clone the berval array into an nsISupportsArray
>+    //
>+    PRUint32 i(0);
I find PRUint32 i = 0; clearer. Matter of style i guess.

You'll need some mac project changes. Let me know if you expect me to do them.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: getBinaryValues patch, v3

OK, this version fixes the problems found by peterv.  If you felt like doing
the mac buildsystem patch, that'd be nifty; but if not, that's ok too.	Lemme
know.  :-)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Bug 125596 has been filed about replacing ns{I}ByteBuffer.

Comment 9

[Michael Buckland]

Mail News team needs info: Stephane, is this a requirement for SMIME?

Comment 10

[Kai Engert (:KaiE:)]

Michael, you asked Stephane a question, but I just realize that he was not on
the CC list.

I think we want this for the beta.
I didn't have time to work on bug 119394 yet, but that's what I want to do in
the next few days.

Comment 11

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

After some discussion with peterv a while back, I became convinced to switch
away from nsISupportsArray and just use an XPCOM array.  Given that most binary
attributes are going to have usually one, sometimes a few, values,
nsILDAPMessage::GetBinaryAttributes is the wrong place to try and optimize using
an nsISupportsArray anyhow.  Conceivably making nsILDAPBERValue return some sort
of array reference might speed things up a bit, but it's unclear whether that
would be particularly noticable.  I'll be attaching a new patch shortly.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: getBinaryValues patch, v4

OK, here we go.  peterv, kaie: whaddya think?

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: getBinaryValues patch, v5

Had to adjust a few headers outside of the XPCOM SDK to cope with #include file
changes, and also made a few tweaks to avoid unnecessarily breaking backwards
compatibility.

Comment 15

[Michael Buckland]

Discussed at mail news bug meeting.  Decided to minus this bug.

Comment 16

[Stephane Saux]

Minusing this bug kills a needed enterprise S/MIME fix.
Can we reconsider?

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Note to self: need to add assertions to check that get*Values is being called on
messages of the ENTRY type and no others.

Comment 18

[Kai Engert (:KaiE:)]

Michael, please have a look at Stephane's answer to your comment. We would like
to ask you to reconsider this bug.

I finally had time to try this patch. It works great! Thanks, Dan!

The code in bug 119394 already works. This bug blocks 119394, and we want 119394
for RTM.

Comment 19

[Kai Engert (:KaiE:)]

Comment on attachment 79222 [details] [diff] [review]
getBinaryValues patch, v5

r=kaie

Comment 20

[Jeff Hooker]

PM chime in: this fnctionality is CRITICAL for S/MIME and RTM.

Comment 21

[Kai Engert (:KaiE:)]

Raising priority as per Jeff's comment.

Comment 22

[Jeff Hooker]

adding jhooker to bug

Comment 23

[scottputterman]

Please don't change bugs to nsbeta1+ or change the impact of bugs that you or
your team don't own.

In order for this to be considered, you'll need to get a super review and get
this baked on the trunk very soon.

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 79222 [details] [diff] [review]
getBinaryValues patch, v5

>Index: directory/xpcom/base/public/nsILDAPMessage.idl
>===================================================================
>RCS file: /cvsroot/mozilla/directory/xpcom/base/public/nsILDAPMessage.idl,v
>retrieving revision 1.12
>diff -u -r1.12 nsILDAPMessage.idl
>--- directory/xpcom/base/public/nsILDAPMessage.idl	20 Sep 2001 07:46:52 -0000	1.12
>+++ directory/xpcom/base/public/nsILDAPMessage.idl	15 Apr 2002 05:32:24 -0000
>@@ -178,4 +180,20 @@
>      * it also returns the closest existing DN to the entry requested.
>      */
>     readonly attribute AString matchedDn;
>+
>+    /**
>+     * Get an array of all the attribute values in this message (a wrapper
>+     * around the LDAP C SDK's get_values_len()).
>+     *
>+     * @param attr      The attribute whose values are to be returned
>+     * @param count     Number of values in the outbound array.
>+     * @param values    Array of nsILDAPBERValue objects
>+     *
>+     * @exception NS_ERROR_UNEXPECTED           Bug or memory corruption
>+     * @exception NS_ERROR_LDAP_DECODING_ERROR  Attribute not found or other 
>+     *                                          decoding error.
>+     * @exception NS_ERROR_OUT_OF_MEMORY
>+     */
>+    void getBinaryValues(in string attr, out unsigned long count,
>+                  [retval, array, size_is(count)] out nsILDAPBERValue values);

Line up the second line to 'in string attr, ...'

>+NS_IMETHODIMP 
>+nsLDAPMessage::GetBinaryValues(const char *aAttr, PRUint32 *aCount,
>+                               nsILDAPBERValue ***aValues)
>+{
>+    struct berval **values;
>+
>+    PR_LOG(gLDAPLogModule, PR_LOG_DEBUG,
>+           ("nsLDAPMessage::GetBinaryValues(): called with aAttr = '%s'", 
>+            aAttr));
>+
>+    values = ldap_get_values_len(mConnectionHandle, mMsgHandle, aAttr);
>+
>+    // bail out if there was a problem
>+    //
>+    if (!values) {
>+        PRInt32 lderrno = ldap_get_lderrno(mConnectionHandle, 0, 0);
>+
>+        if ( lderrno == LDAP_DECODING_ERROR ) {
>+            // this may not be an error; it could just be that the 
>+            // caller has asked for an attribute that doesn't exist.
>+            //
>+            PR_LOG(gLDAPLogModule, PR_LOG_WARNING, 
>+                   ("nsLDAPMessage::GetBinaryValues(): ldap_get_values "
>+                    "returned LDAP_DECODING_ERROR"));
>+            return NS_ERROR_LDAP_DECODING_ERROR;
>+
>+        } else if ( lderrno == LDAP_PARAM_ERROR ) {
>+            NS_ERROR("nsLDAPMessage::GetBinaryValues(): internal error: 1");
>+            return NS_ERROR_UNEXPECTED;
>+
>+        } else {
>+            NS_ERROR("nsLDAPMessage::GetBinaryValues(): internal error: 2");
>+            return NS_ERROR_UNEXPECTED;
>+        }
>+    }
>+
>+    // count the values
>+    //
>+    PRUint32 numVals = ldap_count_values_len(values);
>+
>+    // create the out array
>+    //
>+    *aValues = NS_STATIC_CAST(nsILDAPBERValue **, 
>+                           nsMemory::Alloc(numVals * sizeof(nsILDAPBERValue)));

Line up the second line with 'nsILDAPBERValue **,'

>Index: extensions/pref/autoconfig/src/nsLDAPSyncQuery.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/pref/autoconfig/src/nsLDAPSyncQuery.h,v
>retrieving revision 1.1
>diff -u -r1.1 nsLDAPSyncQuery.h
>--- extensions/pref/autoconfig/src/nsLDAPSyncQuery.h	21 Dec 2001 22:22:01 -0000	1.1
>+++ extensions/pref/autoconfig/src/nsLDAPSyncQuery.h	15 Apr 2002 05:32:30 -0000
>@@ -39,6 +39,7 @@
> 
> #include "nsCOMPtr.h"
> #include "nsILDAPConnection.h"
>+#include "nsILDAPOperation.h"
> #include "nsILDAPMessageListener.h"
> #include "nsILDAPURL.h"
> #include "nsString.h"
>Index: xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.h,v
>retrieving revision 1.10
>diff -u -r1.10 nsLDAPAutoCompleteSession.h
>--- xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.h	5 Oct 2001 02:59:31 -0000	1.10
>+++ xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.h	15 Apr 2002 05:32:30 -0000
>@@ -24,6 +24,7 @@
> #include "nsCOMPtr.h"
> #include "nsIAutoCompleteSession.h"
> #include "nsILDAPConnection.h"
>+#include "nsILDAPOperation.h"
> #include "nsILDAPMessageListener.h"
> #include "nsILDAPAutoCompleteSession.h"
> #include "nsILDAPAutoCompFormatter.h"


Why do you need those two chunks?

r=peterv

Comment 25

[Kai Engert (:KaiE:)]

Attachment: getBinaryValues patch, v6

> Line up the second line to 'in string attr, ...'
> Line up the second line with 'nsILDAPBERValue **,'

done, attaching new patch, only alignments changed.


> Why do you need those two chunks?

It doesn't compile without them. Dan removed
  #include "nsILDAPOperation.idl"
from nsILDAPMessage.idl, and replaced it with an interface forward declaration.

I suspect he did so because of recursion problems.
But because the size of the type is no longer known in source files, you have
to include the header in source files.

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 86187 [details] [diff] [review]
getBinaryValues patch, v6

carrying forward r=peterv

Comment 27

[Kai Engert (:KaiE:)]

Attachment: macbuild patch

The code patch did not yet contain the changes to the Mac build system. They
are now contained in this separate patch.

I don't have a Mac, but I think this patch should work.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: Slightly better macbuild patch

Comment 29

[Peter Van der Beken [:peterv]]

Attachment: Slightly better macbuild patch

Now with all the changes :-)

Comment 30

[Kai Engert (:KaiE:)]

Comment on attachment 86211 [details] [diff] [review]
Slightly better macbuild patch

r=kaie

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 86212 [details] [diff] [review]
Slightly better macbuild patch

r=kaie

Comment 32

[(not reading, please use [e-mail] instead)]

Comment on attachment 86187 [details] [diff] [review]
getBinaryValues patch, v6

sr=sspitzer

some comments:


1)

do you think we need a getAsUTF8()?

dmose says:  "no, not at this point."


2)

can a LDAP BER value be 0 bytes long?
I'm worried about the server (or our code) setting or getting a ber value with
0 bytes.
what will alloc() and memcpy crash in that scenario?
should we make the code check and bail in this scenario?

3)

+// void setFromString(in AUTF8String aValue);
+//
+NS_IMETHODIMP
+nsLDAPBERValue::SetFromUTF8(const nsACString & aValue)

comment doesn't match actual method name.

Comment 33

[(not reading, please use [e-mail] instead)]

Comment on attachment 86212 [details] [diff] [review]
Slightly better macbuild patch

sr=sspitzer

assuming that someone with a mac has built with these changes.

"I don't have a Mac, but I think this patch should work."

those are some scary-tinderbox-goes-red words.

Comment 34

[scottputterman]

Was someone able to build on the Mac?  This should get landed on the trunk and
verified.  If this doesn't get approved for the trunk, then I'd suggest building
a test build and getting QA verification.  I'm not sure if you'll need 119394
before doing this.

Comment 35

[Peter Van der Beken [:peterv]]

I'll be testing this today.

Comment 36

[Peter Van der Beken [:peterv]]

Builds fine on Mac.

Comment 37

[selmer (gone)]

This is a rather enormous patch.  What is being done to ensure adequate test
coverage?  Is it possible to limit any damage to the actual attempted use of
this feature?  (That is, don't have anything bad happen unless an actual binary
attribute exists and is referenced.)

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

selmer: this patch is entirely a new feature, and won't affect any existing
functionality.  The only caller of this code will be the code in bug 119394, and
testing plans for that have been appended to that bug.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: getBinaryValues patch, v7

I tweaked the interface comments a bit, as well as the implementation in
nsLDAPBerValue.cpp to address the zero-length comment.	I talked to mcs, and
zero-length BER values are legit, so this has changed to permit that.  This
means that C++ callers MUST check the size of the array before de-referencing
it.  

kai, can you test this latest version with your patch?

peterv, can you re-review?

Comment 40

[Kai Engert (:KaiE:)]

dmose: I diffed the last two versions of your patch, you are no longer including
the last two hunks to mozilla/extensions/pref/autoconfig/src/nsLDAPSyncQuery.h
and  mozilla/xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.h. (I
guess you forgot them accidentially.)

Note to reviewers: These missing hunks should not stop you from reviewing, they
only contain added #include statements.

Comment 41

[Kai Engert (:KaiE:)]

I tested the latest version of this patch with the patch in bug 119394. It still
works for me.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: getBinaryValues patch, v8

Kai: good catch; thanks!  I did inadvertantly forget to diff those two files. 
Here's a new, corrected patch.

Comment 43

[scottputterman]

The goal is to land 119394 by the end of the week.  I assume this has to go with
it.  Will this be ready with the r=/sr=?

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 87404 [details] [diff] [review]
getBinaryValues patch, v8

r=peterv

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

putterman: as soon as this patch gets final sr (which I've already mailed mscott
for), the patches for both bugs can land on the trunk.

Comment 46

[Scott MacGregor]

Comment on attachment 87404 [details] [diff] [review]
getBinaryValues patch, v8

sr=mscott

Comment 47

[Dan Mosedale (:dmosedale, :dmose)]

Fixed on the trunk.  Still needs to land on the branch.

Comment 48

[Aleksey Nogin]

This checking have added two "might be used uninitialized" warnings:

+directory/xpcom/base/src/nsLDAPBERValue.cpp:59
+ `PRUint8 * array' might be used uninitialized in this function
+
+directory/xpcom/base/src/nsLDAPMessage.cpp:600
+ `nsresult rv' might be used uninitialized in this function

Comment 49

[[e-mail] (gone - use [e-mail] instead)]

This bug, a low-level API, blocked bug 119394, which is now also fixed.

(Meaning that this code works).

I've also checked LXR on the trunk, and all of the files in
http://bugzilla.mozilla.org/attachment.cgi?id=87404&action=view have landed.

Marking VERIFIED for the trunk's state.

Comment 50

[Charles Rosendahl]

I have indeed verified in bug 119394 that binary certs are being downloaded
successfully into the 20020614 Trunk builds and can be used properly.

Comment 51

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 52

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 53

[Dan Mosedale (:dmosedale, :dmose)]

Fixed on the branch.  Bug 152700 has been opened about the warnings; I've got a
patch for them there.

Comment 54

[yulian chang]

Based on Charles Rosendahl's verification on branch build:
http://bugzilla.mozilla.org/show_bug.cgi?id=119394,
Marked this bug fix verified on branch.