168226 - XPCOM Wrapper does not support ldap port numbers greater than 32k

Status: RESOLVED FIXED

(Directory :: LDAP XPCOM SDK, defect)

Description

[John Marmion]

While testing an in-house LDAP server, I observed that mozilla fails to connect
to port number 33400 i.e. a value greater than 32k. The port number is defined
as a short whilst the underlying c-sdk supports an int. There is some further
ambiguity where the Address Book uses  integers and the GetPort() method of
nsILDAPURL also supports integers. I propose that we support integer values as
the ldap c-sdk appears to use 64k as the maximum port value allowed.

I will post a patch if this is agreeable.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

For the life of me, I can't figure out why I did that.  I must have been on
crack.  A patch would be great.  Also, feel free to take this bug if you wish.

Comment 2

[John Marmion]

Attachment: proposed patch to convert port from short to long

Comment 3

[John Marmion]

taking ownership of this bug. 

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 99050 [details] [diff] [review]
proposed patch to convert port from short to long

This looks good for the XPCOM SDK itself, but the patch will also need to
change all the callers in the tree as well, I think.

Comment 5

[John Marmion]

Dan, I am open to correction on this, but i have searched the tree and all the
callers are correctly using the data type PRInt32 for the port value prior to
calling nsLDAPConnection::Init(). This is due to the fact that the GetPort()
method of nsILDAPURL uses PRInt32 and the value of the port is extracted using
that prior to calling the Init(). This includes the following:

directory/xpcom/base/src/nsLDAPService.cpp
mailnews/addrbook/src/nsAbLDAPReplicationQuery.cpp
mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
extensions/pref/autoconfig/nsLDAPSyncQuery.cpp
xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.cpp

Comment 6

[John Marmion]

Assigning this bug to [e-mail]

Comment 7

[Sean Gao]

I am convinced that the callers' code are not affected by this patch as John
said. Thus I will seek for review for this.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 99050 [details] [diff] [review]
proposed patch to convert port from short to long

My sr only applies if you've verified all the callers.

Comment 9

[Gilbert Fang]

wind, please continue the rest work

Comment 10

[wind li]

There are 6 callers in mozilla tree.They are all correctly using the data type 
PRInt32 for the port value prior to calling nsLDAPConnection::Init().They are :

1.directory/xpcom/base/src/nsLDAPService.cpp
caller code:
    rv = conn->Init(host.get(), port, 
                    (options & nsILDAPURL::OPT_SECURE) ? PR_TRUE : PR_FALSE, 
                    nsnull, this);
port define as:
        PRInt32 port;

2.mailnews/addrbook/src/nsAbLDAPReplicationQuery.cpp
caller code:
    // initialize the LDAP connection
    return mConnection->Init(host.get(), port, 
			     (options & nsILDAPURL::OPT_SECURE) ? PR_TRUE
			     : PR_FALSE, PromiseFlatString(aAuthDN).get(),
			     listener);
port define as:
        PRInt32 port;

3.mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp
caller code:
    rv = ldapConnection->Init(host.get(), port, options, nsnull,
                              messageListener);
port define as:
        PRInt32 port;

4.extensions/pref/autoconfig/nsLDAPSyncQuery.cpp
caller code:
    rv = mConnection->Init(host.get(), port, 
                           (options & nsILDAPURL::OPT_SECURE) 
                           ? PR_TRUE : PR_FALSE, 0, selfProxy);
port define as:
        PRInt32 port;
5.xpfe/components/autocomplete/src/nsLDAPAutoCompleteSession.cpp
caller code:
    rv = mConnection->InitLDAP(host.get(), port,
                           (options & nsILDAPURL::OPT_SECURE) ? PR_TRUE 
                           : PR_FALSE, nsnull, selfProxy);
port define as:
        PRInt32 port;

6.directory/xpcom/datasource/nsLDAPDataSource.js
code like this:
    connection.initLDAP(queryURL.host, queryURL.port, null,
            generateGetTargetsBoundCallback())

queryURL is a object of nsILDAPURL,the "port" property is  inherited from 
nsIURL and define as:
    attribute long port;

Comment 11

[wind li]

Peter,
Does your sr apply now?

Comment 12

[Peter Van der Beken [:peterv]]

If those are all the callers in the tree, yes.

Comment 13

[wind li]

Yes,these are all the callers in the tree.
I change the name of function fom Init to InitLDAP,then do a full build.When I 
got an error,I record and modify the caller.I got all c/c++ caller by this way.
And I search all calls to Init function in js/xul files,check every file to see 
if it is a call to nsLDAPConnection.Init.I got only one.

Comment 14

[wind li]

Attachment: update to current trunk

update to current trunk to check in

Comment 15

[Henry Jia]

Comment on attachment 132918 [details] [diff] [review]
update to current trunk

Seems ok.

r=Henry and sr=Peter (according to comment #12)

Comment 16

[wind li]

Has beed checked in.