Closed Bug 1590976 Opened 4 months ago Closed 4 months ago

libprldap returns the wrong error codes

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(thunderbird_esr6871+ fixed, thunderbird71 fixed, thunderbird72 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 71+ fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: benc, Assigned: benc)

References

Details

Attachments

(1 file, 1 obsolete file)

The libprldap library provides wrapper functions for libldap to get and set the error state.
However, it treats NSPR OS error (PR_GetOSError()) as the error state, rather than the NSPR error (PR_GetError()).
This is a problem, because functions can set an NSPR error code along with a zero OS error.
And this means that genuine errors don't get picked up.

(The particular case that highlights this is from Bug 1576364 and screws up LDAP with async sockets. The NSS PR_Send() function correctly sets the PR_WOULD_BLOCK error code while the secure connection is being set up, but with a zero system error code. So libldap doesn't realise that we're still waiting, and and bails out, complaining that there's no connection).

This patch sorts out the error tracking in libprldap.
It uses the NSPR error code (PR_GetError()) rather than the NSPR OS error code (PR_GetOSError()).
libldap still expects to use OS-style errno codes, so there's a conversion. It's not a perfect mapping, but it doesn't need to be (the critical thing is that the async blocking is correctly communicated upward)

Assignee: nobody → benc
Attachment #9103845 - Flags: review?(mkmelin+mozilla)
Blocks: 1576364
Comment on attachment 9103845 [details] [diff] [review]
1590976-fix-libprldap-errno-1.patch

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

Looks alright to me, with some comment nits

::: ldap/c-sdk/libraries/libprldap/ldappr-error.c
@@ +301,5 @@
>      {PR_HOST_UNREACHABLE_ERROR, EHOSTUNREACH},
>      {PR_MAX_ERROR, -1},
>  };
>  
> +/* Set an appropriate NSPR error code for a given os errno. */

For function documentation comment, please use /** */ style.

@@ +303,5 @@
>  };
>  
> +/* Set an appropriate NSPR error code for a given os errno. */
> +void prldap_set_errno(int oserrno) {
> +  /* NOTE: it's the code returned by PR_GetError() which we'll consider the

and for normal comments //

@@ +309,5 @@
> +   * no system code, ie PR_SetError(prerrcode, 0).
> +   * We set the system error here because it'd seem rude not to, but it
> +   * shouldn't be relied upon to check for error states.
> +   */
> +  int i;

just delcare i in the for loop. for (int i = 0; ....

@@ +323,4 @@
>  
> +/* Get the NSPR error code, converted to an appropriate os errno. */
> +int prldap_get_errno(void) {
> +  int i;

here too

@@ +324,5 @@
> +/* Get the NSPR error code, converted to an appropriate os errno. */
> +int prldap_get_errno(void) {
> +  int i;
> +  PRErrorCode nsprerr = PR_GetError();
> +  int oserr = -1; /* unknown */

might as well document it for the function with a @return
Attachment #9103845 - Flags: review?(mkmelin+mozilla) → review+

(In reply to Magnus Melin [:mkmelin] from comment #2)

Comment on attachment 9103845 [details] [diff] [review]

  • /* NOTE: it's the code returned by PR_GetError() which we'll consider the

and for normal comments //

I've left these as /* ... */ just because the rest of the library uses that convention.

just declare i in the for loop. for (int i = 0; ....

Just me showing my age :-). I remember when the scoping rules here varied between compilers...
Fixed.

Attachment #9103845 - Attachment is obsolete: true
Attachment #9104078 - Flags: review+
Keywords: checkin-needed

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a28b4932c562
Fix errno <-> PRErrorCode mapping in libprldap. r=mkmelin

Status: NEW → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Comment on attachment 9104078 [details] [diff] [review]
1590976-fix-libprldap-errno-2.patch

Let's take this with bug 1576364.
Attachment #9104078 - Flags: approval-comm-beta+
Attachment #9104078 - Flags: approval-comm-esr68?
Attachment #9104078 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.