221694 - ber_get_next_buffer_ext() fails when length or tag is split across buffers

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect, P1)

Description

[Mark Smith [:mcs]]

ber_get_next_buffer_ext() is not restartable in the sense that in some cases it
will not pick up correctly where it left off when it is called more than once to
read the same LDAP message.  In particular, there are problems if the length is
more than one byte long and the bytes that make up the length are split across
two buffers that are passed to ber_get_next_buffer_ext() in two different calls.
 A similar problem exists for multi-byte tags (although those are rare) and
ber_get_next() also may have similar bugs.

Comment 1

[Mark Smith [:mcs]]

Noriko is working on this bug.

Comment 2

[Mark Smith [:mcs]]

To cover the ber_get_next() issues I filed bug 222106 - "ber_get_next() fails
when length or tag is split across buffers"

Comment 3

[Noriko Hosoi]

Attachment: proposal for fixing ber_get_next_buffer_ent

If the length is more than one byte long and the bytes that make up the length
are split across the buffer boundary, the byte(s) read in the first
read/receive is stored in ber_buf and combined with the rest read in the second
read/receive, which could be repeated until the complete length info is
received.

Comment 4

[Mark Smith [:mcs]]

Comment on attachment 133294 [details] [diff] [review]
proposal for fixing ber_get_next_buffer_ent

The fix looks good. I will commit this soon.

Comment 5

[Mark Smith [:mcs]]

It is worth noting that the fix does not address potential problems with
multibyte tags. But I am pretty sure that all LDAP tags are one byte long.

Comment 6

[Mark Smith [:mcs]]

Attachment: slightly revised fix (improved comments and eliminated compiler warnings)

Comment 7

[Mark Smith [:mcs]]

Fix committed to the trunk:

mozilla/directory/c-sdk/ldap/libraries/liblber/io.c
  new revision: 5.4; previous revision: 5.3
    Fix bug # 221694 - ber_get_next_buffer_ext() fails when length or tag
                       is split across buffers.

Comment 8

[Mark Smith [:mcs]]

Reopened because we saw some problems that we can't explain.

Comment 9

[Mark Smith [:mcs]]

Attachment: make the ber_get_next_buffer_ext() code more bulletproof

Comment 10

[Mark Smith [:mcs]]

Additional fixes committed to the trunk; reviewed by Noriko:

mozilla/directory/c-sdk/ldap/libraries/liblber/io.c
  new revision: 5.6; previous revision: 5.5
    Additional "bulletproofing" for bug # 221694 - ber_get_next_buffer_ext()
      fails when length or tag is split across buffers.

Comment 11

[Mark Smith [:mcs]]

Marking fixed.

Comment 12

[Mark Smith [:mcs]]

Reopening bug.  Need to more precisely indicate errors. Also, the recent changes
broke some applications that checked *Bytes_Scanned == 0 to determine if a fatal
error occurred; we need to be backward compatible.

Comment 13

[Mark Smith [:mcs]]

Attachment: be backwards compatible; return more precise error info.

Comment 14

[Mark Smith [:mcs]]

Attachment: additional fix to handle zero length messages

Comment 15

[Mark Smith [:mcs]]

Attachment: consolidated patch; ready to commit

Comment 16

[Mark Smith [:mcs]]

Additional fix committed to the trunk; reviewed by Noriko and EY:

mozilla/directory/c-sdk/ldap/libraries/liblber/io.c
  new revision: 5.7; previous revision: 5.6
    Additional fix for 221694 - ber_get_next_buffer_ext() fails when
                                length or tag is split across buffers.
    Improve backwards compatibility by setting *Bytes_Scanned to zero
    when errors occur; use errno to report specific errors.