273013 - short timeouts handled poorly with ldap_result

Status: NEW

(Directory :: LDAP C SDK, defect)

Description

[Steve Hanna]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

If you specify a short timeout with ldap_result (less
than two seconds) and you're fetching search results and the
system clock rolls over to the next second while you're
waiting for the first result, wait4msg times out right
after it fetches that first result but before it loops
to get later results. This can make the actual timeout
tiny (<5 ms) but only when the clock happens to roll over
at just the wrong time.

This is a big problem in applications that require a timeout
less than two seconds but don't want flakey behavior (timeouts
sometimes set to arbitrarily small values).


Reproducible: Always
Steps to Reproduce:
1.
2.
3.



Expected Results:  
Use the timeout actually specified.

This happens because wait4msg uses the time() function
for timeouts. That function has only second granularity.
When NSPR is being used, it should use NSPR's PR_IntervalNow()
which offers millisecond granularity. When NSPR is not
being used, you could use gettimeofday() when available.
Otherwise, you may need to fall back to time().

If you would like to have code for a patch, I'd be
glad to supply it. Using gettimeofday() is simple.
Using PR_IntervalNow() is more complex since it
requires an extra callback.

Comment 1

[Mark Smith [:mcs]]

I think your analysis of the problem is accurate.  We are always happy to
receive patches.  I am not sure it is worth pulling NSPR into the picture, since
gettimeofday() is generally available.  I suspect we would need to use a
different approach on Win32 though.

Comment 2

[Steve Hanna]

(In reply to comment #1)

OK, fine. We'll put together a fix using
gettimeofday() on UNIX/Linux platforms,
GetTickCount() on Windows platforms, and
time() (like the current code) on all
other platforms.

Comment 3

[Jeff Vandegrift]

Attachment: Tar file: bugfixed .c file, test .c file, my build sh script, test sh script, readme

This has been tested on Solaris 8 and we have shipped it to our customer. Our
confidence level is high.

Comment 4

[Jeff Vandegrift]

Attachment: update to the original fix submitted as a tar file

The code change previously submitted as ldap-c-sdk-bug-273013.tar did not
handle a null timeval pointer correctly. This file ldap-c-sdk-bug-273013-v2.tar
supercedes the earlier tar.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 191726 [details]
update to the original fix submitted as a tar file

(I'm going through unassigned review requests on bugzilla) Not sure who owns this code these days, flagging two recent reviewers.

Please reassign reviews as needed, or close if this old bug is no longer relevant.

Comment 6

[Mark Smith [:mcs]]

As far as I know the bug is still relevant.  Unfortunately, I do not know of anyone who is actively maintaining this code.  Rich, does your team still use the LDAP C SDK code?

Comment 7

[Rich Megginson]

(In reply to Mark Smith from comment #6)
> As far as I know the bug is still relevant.  Unfortunately, I do not know of
> anyone who is actively maintaining this code.  Rich, does your team still
> use the LDAP C SDK code?

No, not really.  Not sure who maintains ldap c sdk these days - probably someone on the tbird team.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 191726 [details]
update to the original fix submitted as a tar file

Let's try bouncing the review then. Mark, can you review or otherwise do something to get this out of my requester queue? :)

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 191726 [details]
update to the original fix submitted as a tar file

Ok, I'm not convinced I'm really the right person to review this, assuming the changes are in the TEST_BUG_FIX_FROM_FUNK_SOFTWARE ifdef, then I think I'd like to get someone else to look at these.

However due to the potential changes between when this patch was made and now, I'd really like to get an actual patch for this against the current code.

I realised a lot of time has gone by, but I wonder if Jeff would be able to redo the patch for this, and then we'll get it reviewed.