337210 - ber_scanf sometimes gives incorrect return code on 64-bit

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[Ulf Weltman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

In this example I'm using ber_scanf with format "{aab" (like a MODRDN) when the boolean at the end is missing.  This should return BER_DEFAULT.

When ber_scanf tries to parse the boolean it calls ber_get_boolean, which calls ber_get_int.  ber_get_int will return BER_DEFAULT (0xFFFFFFFFUL) but ber_get_boolean treats the return code as an int.

Then after ber_scanf finishes the initial iteration of the format string it compares rc to LBER_DEFAULT.  This block wont be executed on a 64-bit platform because the comparison will be false after the return code was demoted to an int.

Also, and this is how we noticed it, since rc is also the return code from ber_scanf the calling application wont be able to compare it correctly to LBER_ERROR.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ulf Weltman]

Attachment: fix

Comment 2

[Mark Smith [:mcs]]

Comment on attachment 221390 [details] [diff] [review]
fix

The patch looks good.  Thanks for finding this and providing a fix.

Comment 3

[Rich Megginson]

Fixed.  Thanks Ulf!  Mark, you can reassign to me or just mark as resolved/fixed.
Checking in decode.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/decode.c,v  <--  decode.c
new revision: 5.4; previous revision: 5.3
done

BTW, this is a good candidate for inclusion into the mozilla client branch.  Is there some way I should mark this bug as a merge candidate?

Comment 4

[Mark Smith [:mcs]]

Reassigned to Rich.

I also think this fix should be committed to the client branch (low risk and this fixes a bug that will affect 64bit builds of TBird, etc.)  I don't know if 64bit builds are done by anyone today though.

Dan, any thoughts?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Sure; a=dmose for committing this to the branch!

Comment 6

[Mark Smith [:mcs]]

It does not look like this was committed to ldapcsdk_5_17_client_branch.  Rich, can you check?

Comment 7

[Rich Megginson]

I was not commited to the client 5_17 branch - I'll commit it since dmose approved.