Closed Bug 1562157 Opened 5 years ago Closed 5 years ago

Remove [array] use in xpidl from ldap/

Categories

(MailNews Core :: LDAP Integration, task)

Product:
MailNews Core
Component:
LDAP Integration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6871+ fixed, thunderbird71 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0
Tracking Flags:
Tracking Status
thunderbird_esr68 71+ fixed
thunderbird71 --- fixed

People

(Reporter: mkmelin, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

1562157-part-one-nsILDAPBERValue.patch
8.45 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1562157-part-two-nsILDAPMessage-getAttributes-2.patch
12.32 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1562157-part-three-nsILDAPMessage-leftovers-1.patch
10.52 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1562157-part-four-nsIAbLDAPCard-and-nsILDAPService-1.patch
16.01 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Assignee: nobody → benc

Cleans up the xpidl for nsILDAPBERValue. There's only the one ldap unit test and seems to pass fine.

  • the nsLDAPOperation::CopyValues changes probably need the most scrutiny. The memory handling is pretty hairy, and I'm pretty sure the cleanup code would never have worked previously. The changes
    compile and I've tried to be really careful, but I don't think this function has unit test coverage...
  • I ditched some null-checking on new operators. Going by C++ conventions and looking at examples elsewhere in the code we shouldn't be doing null checks on new, right? There are a few mallocs which I do null check.
  • A clang-format pass asked for left-star types rather than right-star types (ie char* foo rather than char *foo). So I've changed the lines my patch directly touched, but I'm not sure what policy is on this.
Attachment #9085342 - Flags: review?(jorgk)

Just one of three methods in the nsILDAPMessage idl. It's a reasonably big change, so figured it was best to keep the steps small.
Switching to nsTArray<> simplifies nsILDAPMessage::getAttributes() a /lot/, so it's basically a rewrite. But the diff gets pretty noisy trying to map lines from old to new.

:mkmelin: - do you know anything about the encoding rules for LDAP attribute names? There are some clues that they should be treated as utf-8, so that's what I went with.
In nsLDAPSyncQuery::OnLDAPSearchEntry() there's a bit which was treating them as ASCII, and this patch changes it to treat them as utf-8 instead.

Flags: needinfo?(mkmelin+mozilla)
Attachment #9085963 - Flags: review?(jorgk)

Using utf-8 looks alright to me though it's hard to tell in the larger picture if that works or not.

Flags: needinfo?(mkmelin+mozilla)

Sorry, LDAP isn't working properly right now (bug 1574712, bug 1574590), so let's get that fixed first so we can test the changes here.

No problem. There are a few other idl [array] C++ usages to look at while I'm in the swing of it, so I'll move on to them for now.

Comment on attachment 9085342 [details] [diff] [review]
1562157-part-one-nsILDAPBERValue.patch

Review of attachment 9085342 [details] [diff] [review]:
-----------------------------------------------------------------

In one word: Horrible. What a mess for processing an array of bytes. Can't the whole thing be ripped out? Are those get/set functions called a lot? Can't they just be memcpy? I mean why do we have a C++ interface onto something like `berval` which isn't even an object with a destructor, so you need to free its string separately, or forget to do so like the original code did.

Anyway, check bug 1574712 comment #0 for the setup of an LDAP server and then let's test a search in the address book and in auto-complete.

::: ldap/xpcom/src/nsLDAPBERElement.cpp
@@ +80,5 @@
>    if (ber_flatten(mElement, &bv) < 0) {
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  RefPtr<nsLDAPBERValue> berValue = new nsLDAPBERValue();

Why change this to a RefPrt? Used to be a nsCOMPtr.

::: ldap/xpcom/src/nsLDAPBERValue.cpp
@@ +22,2 @@
>  NS_IMETHODIMP
> +nsLDAPBERValue::Get(nsTArray<uint8_t>& aRetVal) {

Nice try moving the & to the left ;-)

::: ldap/xpcom/src/nsLDAPMessage.cpp
@@ +563,5 @@
>    uint32_t i;
>    nsresult rv;
>    for (i = 0; i < numVals; i++) {
>      // create an nsBERValue object
> +    RefPtr<nsLDAPBERValue> berValue = new nsLDAPBERValue();

RefPrt?

::: ldap/xpcom/src/nsLDAPOperation.cpp
@@ +881,5 @@
>    if (!*aBValues) return NS_ERROR_OUT_OF_MEMORY;
>  
>    uint32_t valueIndex;
>    for (valueIndex = 0; valueIndex < valuesCount; ++valueIndex) {
>      nsCOMPtr<nsILDAPBERValue> value(do_QueryElementAt(values, valueIndex, &rv));

Why use rv if we're not checking it?

@@ +908,5 @@
> +  if (*aBValues) {
> +    // Free all the entries we created before the failure.
> +    for (uint32_t i = 0; i < valueIndex; ++i) {
> +      berval* bval = (*aBValues)[i];
> +      free(bval->bv_val);

This free wasn't in the original code. Are we sure this is properly initialised? free(null) is OK these days, right?
Attachment #9085342 - Flags: review?(jorgk) → review+

(In reply to Jorg K (GMT+2) from comment #6)

Comment on attachment 9085342 [details] [diff] [review]
1562157-part-one-nsILDAPBERValue.patch

Review of attachment 9085342 [details] [diff] [review]:

In one word: Horrible. What a mess for processing an array of bytes. Can't
the whole thing be ripped out? Are those get/set functions called a lot?
Can't they just be memcpy? I mean why do we have a C++ interface onto
something like berval which isn't even an object with a destructor, so you
need to free its string separately, or forget to do so like the original
code did.

I haven't looked at the wider context in which this class is used, but I am planning a follow-up patch to replace some of the messy internals with a simple array class. There are a few other places in the xpidl cleanup I'm noting down for possible similar treatment.

Anyway, check bug 1574712 comment #0 for the setup of an LDAP server and
then let's test a search in the address book and in auto-complete.

OK, I'll look into it. I now know waaaaay more about LDAP after this (ie I now know a tiny little bit about LDAP as opposed to absolutely nothing :- )

::: ldap/xpcom/src/nsLDAPBERElement.cpp
@@ +80,5 @@

if (ber_flatten(mElement, &bv) < 0) {
return NS_ERROR_OUT_OF_MEMORY;
}

  • RefPtr<nsLDAPBERValue> berValue = new nsLDAPBERValue();

Why change this to a RefPrt? Used to be a nsCOMPtr.

I changed it to use the concrete type nsLDAPBERValue as opposed to the nsILDAPBERValue interface, and I was under the impression that concrete classes should only be used with RefPtr<>.
(I added a SetRaw() on the concrete class which I need to access here).

+nsLDAPBERValue::Get(nsTArray<uint8_t>& aRetVal) {

Nice try moving the & to the left ;-)

"And I would have got away with it too, if it wasn't for you pesky kids!" :-)

::: ldap/xpcom/src/nsLDAPMessage.cpp
@@ +563,5 @@

uint32_t i;
nsresult rv;
for (i = 0; i < numVals; i++) {
// create an nsBERValue object

  • RefPtr<nsLDAPBERValue> berValue = new nsLDAPBERValue();

RefPrt?

Same again. Concrete class rather than interface.

::: ldap/xpcom/src/nsLDAPOperation.cpp
@@ +881,5 @@

if (!*aBValues) return NS_ERROR_OUT_OF_MEMORY;

uint32_t valueIndex;
for (valueIndex = 0; valueIndex < valuesCount; ++valueIndex) {
nsCOMPtr<nsILDAPBERValue> value(do_QueryElementAt(values, valueIndex, &rv));

Why use rv if we're not checking it?

Good point. Will fix.

@@ +908,5 @@

  • if (*aBValues) {
  • // Free all the entries we created before the failure.
  • for (uint32_t i = 0; i < valueIndex; ++i) {
  •  berval* bval = (*aBValues)[i];

  •  free(bval->bv_val);

This free wasn't in the original code. Are we sure this is properly
initialised? free(null) is OK these days, right?

Yep - I tried to be careful to not add entries to the aBValues list unless they were successfully created, so bv_val should never be null.

I changed it to use the concrete type nsLDAPBERValue as opposed to the nsILDAPBERValue interface, and I was under the impression that concrete classes should only be used with RefPtr<>.

Sorry, I did notice that (and then it slipped my mind again): Old nsCOMPtr<nsILDAPBERValue> berValue = new nsLDAPBERValue();, new: nsCOMPtr<nsILDAPBERValue> berValue = new nsLDAPBERValue(); and then you call SetRaw() on it. OK.

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/63b40a60574a
Remove [array] use in nsILDAPBERValue xpidl. r=jorgk

A new build produces all this, scooby doo would be sad ;)

60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:73:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:126:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:163:11 [-Wsign-compare] comparison of integers of different signs: 'ber_slen_t' (aka 'int') and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:434:42 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:500:18 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:500:41 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:501:17 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:529:16 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:529:39 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:542:18 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:542:41 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:543:17 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:564:16 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:564:39 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:573:44 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/decode.c:598:10 [-Wsign-compare] comparison of integers of different signs: 'ber_int_t' (aka 'int') and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/io.c:662:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/io.c:702:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/io.c:1222:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/liblber/io.c:1363:21 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:394:61 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:409:15 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:423:19 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:431:15 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:432:40 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/error.c:437:13 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/geteffectiverightsctrl.c:89:18 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/getoption.c:419:19 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/getoption.c:450:17 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/getoption.c:457:14 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/getvalues.c:107:10 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/memcache.c:1561:23 [-Wint-to-void-pointer-cast] cast to 'void *' from smaller integer type 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/memcache.c:1563:65 [-Wint-to-void-pointer-cast] cast to 'void *' from smaller integer type 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/os-ip.c:361:66 [-Wpointer-sign] passing 'int *' to parameter of type 'socklen_t *' (aka 'unsigned int *') converts between pointers to integer types with different sign
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/os-ip.c:569:37 [-Wsign-compare] comparison of integers of different signs: 'nsldapi_in_addr_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/proxyauthctrl.c:81:18 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/proxyauthctrl.c:128:18 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/pwmodext.c:74:20 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/pwmodext.c:81:22 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/pwmodext.c:90:22 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/pwmodext.c:99:22 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/pwmodext.c:107:20 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/request.c:488:9 [-Wunused-variable] unused variable 'logname'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/request.c:1363:10 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/saslbind.c:788:10 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/saslbind.c:796:10 [-Wsign-compare] comparison of integers of different signs: 'int' and 'ber_tag_t' (aka 'unsigned int')
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/vlistctrl.c:99:18 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/vlistctrl.c:109:20 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libldap/vlistctrl.c:119:20 [-Wsign-compare] comparison of integers of different signs: 'ber_tag_t' (aka 'unsigned int') and 'int'
60:37.26 warning: comm/ldap/c-sdk/libraries/libprldap/ldappr-io.c:269:21 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'
60:37.26 warning: comm/ldap/c-sdk/libraries/libprldap/ldappr-io.c:285:21 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long'

Are these new? LDAP always produced a heap of warnings.

Flags: needinfo?(benc)

I recall an effort (mostly aceman iirc) to get rid of simple ones like Wsign-compare quite a while ago. It may also be more noticeable due to summarization by the build system at the end.

It may be old: Bug 1243121 and Bug 1277602.

I don't think the warnings are related to these changes (I've not touched the ldap c-sdk).
But I'm definitely all for tightening up types and removing warnings.
What's the provenance of the LDAP C sdk? Is there some upstream version we are (or should) be tracking?
(but probably more a discussion for Bug 1243121, it seems)

Flags: needinfo?(benc)

AFAIK there's not really an upstream for LDAP.

Just minor tweaks to the patch - got rid of that unused rv and ran it all through clang-format with the C-C rules (caught a couple of sneaky right-pointer-alignments).

Also took it through the STR on bug 1574712 comment #0 (The "Alvarez" test :- ) and it all worked fine.

Attachment #9085963 - Attachment is obsolete: true
Attachment #9085963 - Flags: review?(jorgk)
Attachment #9090564 - Flags: review?(jorgk)
Blocks: 1579288

Fixes up nsILDAPService (the last remaining interface in /ldap), and nsIAbLDAPCard in addrbook (because it was easier to fix rather than work around).

I didn't find any javascript that relied on the changed methods.

Still passes the Alvarez test.

Try run here (which all looks pretty sane I think):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9b02ede35cff68bbcdaff202309ddfa57827cf7

Attachment #9090902 - Flags: review?(jorgk)
Comment on attachment 9090564 [details] [diff] [review]
1562157-part-two-nsILDAPMessage-getAttributes-2.patch

What a total mess :-( - `IterateAttributes(aAttrCount, aAttributes, false)` to count first, then do it for real.

Your rewrite looks fine. Sorry about the delay, I've been doing release almost every day, that's a good excuse. Plus this stuff isn't strictly enchanting.
Attachment #9090564 - Flags: review?(jorgk) → review+
Comment on attachment 9090651 [details] [diff] [review]
1562157-part-three-nsILDAPMessage-leftovers-1.patch

Review of attachment 9090651 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK.

::: ldap/xpcom/src/nsLDAPMessage.cpp
@@ +347,2 @@
>      else
> +      aValues.AppendElement(NS_ConvertASCIItoUTF16(sValue));

Encoding experts know that NS_ConvertASCIItoUTF16 also works for Latin-1 (windows-1252). So if it's not UTF-8 than it's 100% not ASCII either, so it had better be Latin-1 or we return garbage here.
Attachment #9090651 - Flags: review?(jorgk) → review+
Comment on attachment 9090902 [details] [diff] [review]
1562157-part-four-nsIAbLDAPCard-and-nsILDAPService-1.patch

Looks OK.
Attachment #9090902 - Flags: review?(jorgk) → review+
Comment on attachment 9090902 [details] [diff] [review]
1562157-part-four-nsIAbLDAPCard-and-nsILDAPService-1.patch

Review of attachment 9090902 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbLDAPCard.cpp
@@ +173,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP nsAbLDAPCard::BuildRdn(nsIAbLDAPAttributeMap *aAttributeMap,
> +                                     nsTArray<nsCString> const &aAttributes,

Hmm, here you leave the variable name aAttributes, but in the earlier patch you shortened it to attrs. I don't see the consistency there. Maybe I'll change it back in the other patch.
Version: 44 → Trunk
Keywords: leave-open

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/da918af75788
Remove [array] use in xpidl for nsILDAPBERMessage.getAttributes(). r=jorgk
https://hg.mozilla.org/comm-central/rev/28e58c1b88e1
Remove remaining [array] use in nsILDAPBERMessage xpidl. r=jorgk
https://hg.mozilla.org/comm-central/rev/821c6838bcd0
Remove xpidl [array] use from nsIAbLDAPCard and nsILDAPService. r=jorgk

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Blocks: 1604773
No longer blocks: 1604773
Regressions: 1604773
Blocks: 1675037
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: