Re: (ITS#8028) NULL pointer dereference in ldap_new_connection()
by hyc＠symas.com
rohanskurane(a)gmail.com wrote:
> Full_Name: Rohan Kurane
> Version: 2.4.40
> OS: BSD 7.2
> URL: ftp://ftp.openldap.org/incoming/
> Submission from: (NULL) (64.80.217.3)
>
>
> In ldap_new_connection() in request.c, while setting up a connection to the LDAP
> server, there is a possibility of dereferencing a NULL pointer in
> lc->locnn_server
Fixed in git master. Please don't send HTML emails, they're particularly
unreadable with embedded code like this.
>
> if ( connect ) {
> LDAPURLDesc **srvp, *srv = NULL;
>
> async % LDADAP_BOOL_GET( &ld->ld_options, LDAP_BOOL_CONNECT_ASYNC );
>
> for ( srvp = srvlist; *srvp != NULL; srvp = &(*srvp)->lud_next ) {
> int rc;
>
> rc = ldap_int_open_connection( ld, lc, *srvp, async );
> if ( rc != -1 ) {
> srv = *srvp;
>
> 9%9 if ( ld->ld_urllist_proc && ( !async || rc != -2 ) ) {
> ld->ld_urllist_proc( ld, srvlist, srvp, ld->ld_urllist_params );
> }
>
> break;
> }
> }
>
> if ( srv == NULL ) {
> if ( !use_ldsb ) {
> ber_sockbuf_free( lc->lconn_sb );
> %%D
> LDAP_FREE( (char *)lc );
> ld->ld_errno = LDAP_SERVER_DOWN;
> return( NULL );
> }
>
> lc->lconn_server = ldap_url_dup( srv );
> }
>
> ldap_url_dup() does a bunch of malloc's to set up lc->lconn_server. If any of
> those malloc's fail, it returns NULL. The code does not check for a NULL
> lconn_server pointer and tries to reference lud_exts. That can cause a
> segmentation fault.
>
> if ( connect ) {
> #ifdef HAVE_TLS
> if ( lc->lconn_server->lud_exts ) {
> int rc, ext = find_tls_ext( lc->lconn_server );
> if ( ext ) {
> LDAPConn *savedefconn;
>
> Even thou this should not happen, is this a known issue and are there any plans
> to fix the openldap library ?
>
> Thank you
>
>
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
8 years, 8 months
(ITS#8028) NULL pointer dereference in ldap_new_connection()
by rohanskurane＠gmail.com
Full_Name: Rohan Kurane
Version: 2.4.40
OS: BSD 7.2
URL: ftp://ftp.openldap.org/incoming/
Submission from: (NULL) (64.80.217.3)
In ldap_new_connection() in request.c, while setting up a connection to the LDAP
server, there is a possibility of dereferencing a NULL pointer in
lc->locnn_server
if ( connect ) {
LDAPURLDesc **srvp, *srv = NULL;
async % LDADAP_BOOL_GET( &ld->ld_options, LDAP_BOOL_CONNECT_ASYNC );
for ( srvp = srvlist; *srvp != NULL; srvp = &(*srvp)->lud_next ) {
int rc;
rc = ldap_int_open_connection( ld, lc, *srvp, async );
if ( rc != -1 ) {
srv = *srvp;
9%9 if ( ld->ld_urllist_proc && ( !async || rc != -2 ) ) {
ld->ld_urllist_proc( ld, srvlist, srvp, ld->ld_urllist_params );
}
break;
}
}
if ( srv == NULL ) {
if ( !use_ldsb ) {
ber_sockbuf_free( lc->lconn_sb );
%%D
LDAP_FREE( (char *)lc );
ld->ld_errno = LDAP_SERVER_DOWN;
return( NULL );
}
lc->lconn_server = ldap_url_dup( srv );
}
ldap_url_dup() does a bunch of malloc's to set up lc->lconn_server. If any of
those malloc's fail, it returns NULL. The code does not check for a NULL
lconn_server pointer and tries to reference lud_exts. That can cause a
segmentation fault.
if ( connect ) {
#ifdef HAVE_TLS
if ( lc->lconn_server->lud_exts ) {
int rc, ext = find_tls_ext( lc->lconn_server );
if ( ext ) {
LDAPConn *savedefconn;
Even thou this should not happen, is this a known issue and are there any plans
to fix the openldap library ?
Thank you
8 years, 8 months
Re: (ITS#8027) ldapsearch -E deref=member: crashes slapd
by hyc＠symas.com
ryan(a)nardis.ca wrote:
> Full_Name: Ryan Tandy
> Version: master (7df548d), RE24 (2b14bbc)
> OS: Debian unstable
> URL:
> Submission from: (NULL) (142.32.208.227)
>
>
> If you use the deref control but leave the list of requested attributes empty,
> slapd crashes.
>
> ldapsearch [...] -E deref=member:
> The ldapsearch manpage implies this probably isn't valid, but it still accepted
> it. (FWIW, I tried it just to see whether it would return all attributes or
> none.) I couldn't tell from draft-ldap-deref-00 whether an empty attr list is
> considered a valid request.
>
Patched in master to reject a request with an empty attr list.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
8 years, 8 months
Re: (ITS#8027) ldapsearch -E deref=member: crashes slapd
by michael＠stroeder.com
hyc(a)symas.com wrote:
> As I read the grammar in the draft section 2.2 the attributeList is not
> OPTIONAL so this is definitely not a valid request.
But this invalid request must not crash slapd with slapo-deref installed.
Ciao, Michael.
8 years, 8 months
Re: (ITS#8027) ldapsearch -E deref=member: crashes slapd
by hyc＠symas.com
ryan(a)nardis.ca wrote:
> Full_Name: Ryan Tandy
> Version: master (7df548d), RE24 (2b14bbc)
> OS: Debian unstable
> URL:
> Submission from: (NULL) (142.32.208.227)
>
>
> If you use the deref control but leave the list of requested attributes empty,
> slapd crashes.
>
> ldapsearch [...] -E deref=member:
>
> #0 0x0000000000516ef0 in deref_parseCtrl (op=0x7fffec000940, rs=0x7ffff57eeac0,
> ctrl=0x7fffec001238) at deref.c:225
> #1 0x000000000046a84d in slap_parse_ctrl (op=0x7fffec000940, rs=0x7ffff57eeac0,
> control=0x7fffec001238, text=0x7ffff57eeae0)
> at controls.c:693
> #2 0x000000000046b0f5 in get_ctrls2 (op=0x7fffec000940, rs=0x7ffff57eeac0,
> sendres=1, ctag=160) at controls.c:886
> #3 0x000000000046a8ff in get_ctrls (op=0x7fffec000940, rs=0x7ffff57eeac0,
> sendres=1) at controls.c:723
> #4 0x000000000042e94e in do_search (op=0x7fffec000940, rs=0x7ffff57eeac0) at
> search.c:195
> #5 0x000000000042bdf3 in connection_operation (ctx=0x7ffff57eebf0,
> arg_v=0x7fffec000940) at connection.c:1134
> #6 0x000000000042c3a3 in connection_read_thread (ctx=0x7ffff57eebf0, argv=0xb)
> at connection.c:1280
> #7 0x0000000000538938 in ldap_int_thread_pool_wrapper (xpool=0x892bc0) at
> tpool.c:958
> #8 0x00007ffff79b00a4 in start_thread (arg=0x7ffff57ef700) at
> pthread_create.c:309
> #9 0x00007ffff76e4ccd in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
>
> (line numbers are from master)
>
> The ldapsearch manpage implies this probably isn't valid, but it still accepted
> it. (FWIW, I tried it just to see whether it would return all attributes or
> none.) I couldn't tell from draft-ldap-deref-00 whether an empty attr list is
> considered a valid request.
As I read the grammar in the draft section 2.2 the attributeList is not
OPTIONAL so this is definitely not a valid request.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
8 years, 8 months
(ITS#8027) ldapsearch -E deref=member: crashes slapd
by ryan＠nardis.ca
Full_Name: Ryan Tandy
Version: master (7df548d), RE24 (2b14bbc)
OS: Debian unstable
URL:
Submission from: (NULL) (142.32.208.227)
If you use the deref control but leave the list of requested attributes empty,
slapd crashes.
ldapsearch [...] -E deref=member:
#0 0x0000000000516ef0 in deref_parseCtrl (op=0x7fffec000940, rs=0x7ffff57eeac0,
ctrl=0x7fffec001238) at deref.c:225
#1 0x000000000046a84d in slap_parse_ctrl (op=0x7fffec000940, rs=0x7ffff57eeac0,
control=0x7fffec001238, text=0x7ffff57eeae0)
at controls.c:693
#2 0x000000000046b0f5 in get_ctrls2 (op=0x7fffec000940, rs=0x7ffff57eeac0,
sendres=1, ctag=160) at controls.c:886
#3 0x000000000046a8ff in get_ctrls (op=0x7fffec000940, rs=0x7ffff57eeac0,
sendres=1) at controls.c:723
#4 0x000000000042e94e in do_search (op=0x7fffec000940, rs=0x7ffff57eeac0) at
search.c:195
#5 0x000000000042bdf3 in connection_operation (ctx=0x7ffff57eebf0,
arg_v=0x7fffec000940) at connection.c:1134
#6 0x000000000042c3a3 in connection_read_thread (ctx=0x7ffff57eebf0, argv=0xb)
at connection.c:1280
#7 0x0000000000538938 in ldap_int_thread_pool_wrapper (xpool=0x892bc0) at
tpool.c:958
#8 0x00007ffff79b00a4 in start_thread (arg=0x7ffff57ef700) at
pthread_create.c:309
#9 0x00007ffff76e4ccd in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(line numbers are from master)
The ldapsearch manpage implies this probably isn't valid, but it still accepted
it. (FWIW, I tried it just to see whether it would return all attributes or
none.) I couldn't tell from draft-ldap-deref-00 whether an empty attr list is
considered a valid request.
8 years, 8 months
(ITS#8026) net-nds/openldap - powerpc-apple-darwin9-gcc: Undefined symbols for architecture ppc: "_slapi_pblock_set"
by daniel.ibnzayd＠inquisitor.com
Full_Name: Daniel Ibn Zayd
Version: 2.4.38-r2
OS: PPC
URL: https://bugs.gentoo.org/show_bug.cgi?id=536306
Submission from: (NULL) (166.84.1.2)
Both of the latest builds marked stable for ppc (openldap-2.4.35-r1 and
openldap-2.4.38-r2) error out when they get to the libaddrdnvalues-plugin:
* Building contrib-module: addrdnvalues plugin
Undefined symbols for architecture ppc:
"_slapi_pblock_set", referenced from:
_addrdnvalues_preop_init in cclXT5Cy.o
"_slapi_log_error", referenced from:
_addrdnvalues_preop_init in cclXT5Cy.o
_addrdnvalues_preop_add in cclXT5Cy.o
"_slapi_pblock_get", referenced from:
_addrdnvalues_preop_add in cclXT5Cy.o
"_slapi_entry_add_rdn_values", referenced from:
_addrdnvalues_preop_add in cclXT5Cy.o
"_slapi_send_ldap_result", referencefrfrom:
_addrdnvalues_preop_add in cclXT5Cy.o
"_ldap_err2string", referenced from:
_addrdnvalues_preop_add in cclXT5Cy.o
ld: symbol(s) not found for architecture ppc
I've researched the plug-in and its components searching for possible fixes but
have not turned up anything. Any clues here are greatly appreciated.
PS: This is a Gentoo Prefix set up I am working with.
https://bugs.gentoo.org/show_bug.cgi?id=536306
8 years, 8 months
Re: (ITS#7971) LMDB: Uncarefully appointment when beginning a readonly txn.
by h.b.furuseth＠usit.uio.no
On 18/10/14 00:46, leo(a)yuriev.ru wrote:
> (...)
> - ti->mti_readers[i].mr_pid = pid;
> - ti->mti_readers[i].mr_tid = tid;
> + r = &ti->mti_readers[i];
> + r->mr_txnid = (txnid_t)-1;
> + r->mr_tid = tid;
> + r->mr_pid = pid; /* should be written last, see ITS#. */
> if (i == nr)
> ti->mti_numreaders = ++nr;
> /* Save numreaders for un-mutexed mdb_env_close() */
> env->me_numreaders = nr;
Actually that too is too early to set mr_pid: We must be
sure env_close() will reset it. But it is also too late,
we must get rid of any garbage value in md_pid at once
to protect it from other processes' env_close(). Fixing.
Also, I'll rename the confusingly named me_numreaders to
me_close_readers and drop some confused code using it.
8 years, 8 months
Re: (ITS#7969) LMDB: Globally shared fields of meta-data are not 'volatile'.
by hyc＠openldap.org
This discussion should be moved to the openldap-devel list as there's
more being discussed now than the patch itself.
Леонид Юрьев wrote:
> 2015-01-14 8:49 GMT+03:00 Hallvard Breien Furuseth <h.b.furuseth(a)usit.uio.no>:
>> On 13/01/15 19:23, Hallvard Breien Furuseth wrote:
>>>
>>> Yes, that didn't come out right. I don't mind inserting the
>>> volatile, but I don't know that it helps either. As far as I
>>> understand if it was broken without volatile, then it's still
>>> broken with it - just hopefully less likely to break. And LMDB
>>> couldn't be releaseed with your original unportable sync code
>>> along with the volatile.
>>
>>
>> Sorry, nevermind. Of course when the writer does sync well enough,
>> volatile + the txn_renew loop will have to do for a sync primitive in
>> the reader.
>
>> I suppose this requires that sync in the writer thread
>> will shake other threads as well, it won't be private to the writer.
>
> In general this is wrong, more precisely depend on 'volatile' on
> shared variables and usage of barriers/fences by the readers.
Actually 'volatile' is meaningless at the hardware level. It only serves
to prevent the compiler from reordering or eliminating accesses. In the
case of LMDB the compiler cannot eliminate accesses because they are to
global memory, and the compiler cannot determine anything about the
liveness or side-effects of global memory accesses.
> Sync-ops due lock/release a mutex by writer issue a memory-barrier for
> its own thread.
> With this compiler must update all modified variables, which shaded in
> the CPU registers.
> Next a hardware write-barrier (aka release) in the mutex-release code
> enforce all changes to be visible for other threads (e.g. flush the
> cache).
> But 'be visible' here mean 'publish' and other threads can access
> these changes, but if want this.
> In general, to see changes made by the writer, all other threads
> should issue a read-barrier (aka acquire).
> On most arches such barrier just inform compiler that memory was
> changed and variables which cached in the registers must be reloaded.
> But in some cases (like Itanium) this barrier will be taken in account
> for instruction scheduling.
> For 'volatile' compiler should generate barriers each time on read or
> write such variables.
No. volatile doesn't generate hardware barriers.
> More general, memory-barriers are very important to HPC, distributed
> computing and for super-computers.
> For example read-barriers may pull changes from internode-bus or other
> nodes, and write-barriers - publish the local changes.
> So, the one way to avoid a race bugs - thinking in terms of
> publish/pull changes.
"race bug" by definition occurs when multiple writers may modify a
particular memory object, causing its value to be indeterminate to an
observer. By definition, no such bugs can occur in LMDB because only
single writers can ever modify any memory object.
In the case of arbitrary readers viewing writes, these are the possible
cases:
1) reader is on same CPU as writer, writes cached
there is no issue, the reader sees what the writer wrote.
2) reader is on same CPU as writer, writes not cached
there is no issue, the reader must fetch data from global memory
3) reader is on different CPU, writes not cached
there is no issue, the reader's CPU must fetch the data from
global memory - same as (2)
4) reader is on different CPU, writes are cached
the reader may see the cached/stale data, or the CPU may fetch
the new data from global memory
Only case (4) has any ambiguity, and LMDB's reader table is specifically
designed not to care about the ambiguity. I.e., whether fresh or stale
data is seen is irrelevant, LMDB will operate correctly because it does
not fresh data. Correct processing of the reader table only depends on
the oldest data in the table, so staleness is an asset here.
Correct processing of changes to the meta page requires exclusion from
other writers, so the write mutex is used. This also guarantees that all
changes to the meta page are flushed to global memory before the next
writer begins.
In ITS#7970 you discuss a "heisenbug" which can theoretically occur if
multiple write txns complete in the span of 2 memory accesses by a
reader. In practice such a bug cannot occur because the act of
completing a write txn involves multiple blocking calls (I/O system
calls, mutex acquisition/release, etc.) which will force writers to
pause relative to any readers in question. In contrast the reader
performs no blocking calls at all, and in the window of vulnerability it
is performing only 2 single-word memory accesses.
Demonstrating the bug by manually inserting yield()s only proves the
point - without those manually inserted yields you cannot actually
trigger any situation where the reader thread will be descheduled
between those two instructions.
You discuss the ramifications of such a bug as the writer potentially
overwriting pages that the reader needs. None of this can actually occur
in current LMDB code, again by design. The fact that you encountered
these issues while debugging your LIFO patch only reflects on the
problems I already pointed out with the LIFO approach.
Hallvard and I had this exact same discussion a few years ago; his
example used the debugger to pause the reader at that point. Certainly,
if you go out of your way to manually halt the reader at a specific
instruction you can break the reader. Without outside intervention it
won't happen.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/
8 years, 8 months
