[Date Prev][Date Next] [Chronological] [Thread] [Top]
Re: FW: ldapsearch problems (ITS#2490)

To: openldap-devel@OpenLDAP.org
Subject: Re: FW: ldapsearch problems (ITS#2490)
From: "Kurt D. Zeilenga" <Kurt@OpenLDAP.org>
Date: Wed, 21 May 2003 14:38:40 -0700
In-reply-to: <00c101c31f30$da015ec0$0e01a8c0@CELLO>
Like Howard noted in a previous post, I am bit concerned by the
"rewrite" approach here.  The question I guess is whether or
not a simple patch could fix the problem or not.  I haven't
yet thought about, if a rewrite was needed, whether or not
this would be most appropriate rewrite.  If we're going to
rewrite things, there may be other problems we should consider.

Anyways, I haven't yet examined the patch yet.  I first need
to understand the underlying problem which let to the creation
of the patch.  I'll try to find some time tomorrow to do that.

Kurt

At 05:35 PM 5/20/2003, you wrote:
>I'm a bit pressed for time, can someone else look this over/comment?
>
>  -- Howard Chu
>  Chief Architect, Symas Corp.       Director, Highland Sun
>  http://www.symas.com               http://highlandsun.com/hyc
>  Symas: Premier OpenSource Development and Support
>
>-----Original Message-----
>From: Patrick Dreyer, SY-UCP [mailto:Patrick.Dreyer@swisscom-ucp.com]
>Sent: Tuesday, May 20, 2003 1:32 AM
>To: Howard Chu; openldap-its@OpenLDAP.org
>Subject: RE: ldapsearch problems (ITS#2490)
>
>
>Hi
>
>> Thanks for this, but it has some obvious problems. Setting errno when
>read() returns 0 is incorrect behavior. This will
>> cause ber_get_next() to treat an EOF/close as a retry.
>
>Yes, you're right. I corrected that in my second solution below.
>
>> Also, even if your patch were valid, you have used C++ style comments in
>it, which is not acceptable.
>
>Sorry for this. It seams the MS compiler is too dump to tell me that, beside
>I should have known it.
>
>As I told you, I want to optimize my first solution and below it is, hopping
>my comments are kind of help...
>Give me your feedpack one more time please and tell me if I should have
>forgotten to take out any C++ code.
>
>Patrick
>
>
>====================================================
>
>/*
> * A rewrite of ber_get_next that can safely be called multiple times for the
> * same packet and will simply continue where it stopped last time until a
>full
> * packet is read.
> *
> * Any ber element looks like this: tag length contents.
> * Assuming everything's ok, we return the tag byte (we can assume a single
> * byte), return the length in len, and the rest of the undecoded element in
> * buf.
> *
> * Assumptions:
> *  1) multi-byte tag and len at most 32 bits wide
> *  2) definite lengths
> *  3) primitive encodings used whenever possible
> *
> * The first few bytes of the message are read to check for multi-byte tags
>and
> * lengths. These bytes are temporarily stored in the ber_usertag and ber_buf
> * fields, called temp buffer, of the berelement until tag/length parsing is
> * complete.
> * ber_ptr is used to indicate if we are parsing the tag or the length and if
> * they are multi-byte to know which byte we are reading.
> * ber_rwptr points to the next byte in the temp buffer and ber_end points to
> * the byte one after the last in the temp buffer.
> *
> * The diagram below shows you how it looks like if we could fill the whole
> * temp buffer before we begin parsing the tag:
> *
> * +-------------+-------------+-------------+-------------+-------------+---
> * |   ber_tag   |   ber_len   | ber_usertag |   ber_buf   |   ber_ptr   | ..
> * +-------------+-------------+-------------+-------------+-------------+--
> * ^                           ^                           ^
> * ber_ptr                  ber_rwptr                   ber_end
> *
> *
> * The following diagram shows you how it could look like if we parsed the
>tag
> * and the length is a multi-byte two bytes long:
> *
> * +-------------+-------------+-------------+-------------+-------------+---
> * |   ber_tag   |   ber_len   | ber_usertag |   ber_buf   |   ber_ptr   | ..
> * +-------------+-------------+-------------+-------------+-------------+--
> *                      ^             ^                    ^
> *                    ber_ptr      ber_rwptr            ber_end
> *
> *
> * After tag/length parsing, any leftover bytes and the rest of the message
>is
> * copied into ber_buf.
> */
>ber_tag_t
>ber_get_next(
>  Sockbuf *sb,
>  ber_len_t *len,
>  BerElement *ber )
>{
>  assert( sb != NULL );
>  assert( len != NULL );
>  assert( ber != NULL );
>
>  assert( SOCKBUF_VALID( sb ) );
>  assert( LBER_VALID( ber ) );
>
>  #ifdef NEW_LOGGING
>    LDAP_LOG( BER, ENTRY, "ber_get_next: enter\n", 0, 0, 0 );
>  #else
>    ber_log_printf( LDAP_DEBUG_TRACE, ber->ber_debug, "ber_get_next\n" );
>  #endif
>
>  /* we start reading the next element */
>  if (ber->ber_rwptr == NULL) {
>    #if 0
>      /* XXYYZ - dtest does like this assert.  */
>      assert( ber->ber_buf == NULL );
>    #endif
>    ber->ber_tag   = 0;
>    ber->ber_len   = 0;
>    ber->ber_ptr   = (char*)&ber->ber_tag;
>    ber->ber_end   = (char*)&ber->ber_usertag;
>    ber->ber_rwptr = (char*)&ber->ber_usertag;
>  }
>
>  /* parse the tag and length */
>  if ((char*)&ber->ber_tag <= ber->ber_ptr
>      && ber->ber_ptr < (char*)&ber->ber_usertag) {
>    /* read more data if we have free space in the temp buffer */
>    {
>      ber_len_t sblen = (char*)&ber->ber_ptr - ber->ber_end;
>
>      /* jump to the beginning of the temp buffer if it is empty and we
>         reached the end */
>      if (sblen == 0 && ber->ber_rwptr == ber->ber_end) {
>        ber->ber_end = (char*)&ber->ber_usertag;
>        sblen        = (char*)&ber->ber_ptr - ber->ber_end;
>      }
>
>      /* read as much bytes as we have space left in the temp buffer */
>      if (sblen > 0) {
>        sblen = ber_int_sb_read(sb, ber->ber_end, sblen);
>        if (sblen <= 0)
>          return LBER_DEFAULT;
>
>        ber->ber_end += sblen;
>
>        /* be sure we have at least one available byte in the temp buffer */
>        if (ber->ber_rwptr == ber->ber_end) {
>          #if defined(EWOULDBLOCK)
>            errno = EWOULDBLOCK;
>          #elif defined(EAGAIN)
>            errno = EAGAIN;
>          #endif
>          return LBER_DEFAULT;
>        }
>      }
>    } /* read more data */
>
>    /* parse the first tag byte */
>    if (ber->ber_ptr == (char*)&ber->ber_tag) {
>      ber->ber_tag = *ber->ber_rwptr;
>
>      /* tag is only one byte long */
>      if ((*ber->ber_rwptr++ & LBER_BIG_TAG_MASK) != LBER_BIG_TAG_MASK)
>        ber->ber_ptr = (char*)&ber->ber_len;
>      else
>        ++ber->ber_ptr;
>    }
>
>    /* parse more tag bytes */
>    if (ber->ber_ptr < (char*)&ber->ber_len) {
>      do {
>        /* be sure the temp buffer is not empty */
>        if (ber->ber_rwptr == ber->ber_end) {
>          #if defined(EWOULDBLOCK)
>            errno = EWOULDBLOCK;
>          #elif defined(EAGAIN)
>            errno = EAGAIN;
>          #endif
>          return LBER_DEFAULT;
>        }
>
>        /* be sure the tag is not too big */
>        if (ber->ber_ptr++ == (char*)&ber->ber_len) {
>          errno = ERANGE;
>          return LBER_DEFAULT;
>        }
>
>        ber->ber_tag <<= 8;
>        ber->ber_tag |=  *ber->ber_rwptr;
>      } while ((*ber->ber_rwptr++ & LBER_MORE_TAG_MASK) ==
>LBER_MORE_TAG_MASK);
>
>      ber->ber_ptr = (char*)&ber->ber_len;
>    } /* parse more tag bytes */
>
>    /* parse the first length byte */
>    if (ber->ber_ptr == (char*)&ber->ber_len) {
>      /* be sure the temp buffer is not empty */
>      if (ber->ber_rwptr == ber->ber_end) {
>        #if defined(EWOULDBLOCK)
>          errno = EWOULDBLOCK;
>        #elif defined(EAGAIN)
>          errno = EAGAIN;
>        #endif
>        return LBER_DEFAULT;
>      }
>
>      /* length is only one byte long */
>      if ((*ber->ber_rwptr & 0x80) == 0) {
>        ber->ber_len = *ber->ber_rwptr++;
>        ber->ber_ptr = (char*)&ber->ber_usertag;
>      }
>      else {
>        ber->ber_len = *ber->ber_rwptr++ & 0x7f;
>
>        /* be sure the length is not too big */
>        if (ber->ber_len > sizeof(ber->ber_len)) {
>          errno = ERANGE;
>          return LBER_DEFAULT;
>        }
>
>        ber->ber_ptr += sizeof(ber->ber_len) - ber->ber_len;
>      }
>    } /* parse the first length byte */
>
>    /* parse more length bytes */
>    for (; ber->ber_ptr < (char*)&ber->ber_usertag; ++ber->ber_ptr) {
>      /* be sure the temp buffer is not empty */
>      if (ber->ber_rwptr == ber->ber_end) {
>        #if defined(EWOULDBLOCK)
>          errno = EWOULDBLOCK;
>        #elif defined(EAGAIN)
>          errno = EAGAIN;
>        #endif
>        return LBER_DEFAULT;
>      }
>
>      ber->ber_len <<= 8;
>      ber->ber_len |=  *ber->ber_rwptr++;
>    }
>
>    /* be sure we have to read at least one byte of data */
>    if (ber->ber_len == 0) {
>      errno = ERANGE;
>      return LBER_DEFAULT;
>    }
>
>    /* be sure sockbuf_max_incoming didn't exceed */
>    if  (sb->sb_max_incoming > 0 && ber->ber_len > sb->sb_max_incoming) {
>      #ifdef NEW_LOGGING
>        LDAP_LOG( BER, ERR,
>          "ber_get_next: sockbuf_max_incoming exceeded "
>          "(%d > %d)\n", ber->ber_len, sb->sb_max_incoming, 0 );
>      #else
>        ber_log_printf( LDAP_DEBUG_CONNS, ber->ber_debug,
>          "ber_get_next: sockbuf_max_incoming exceeded "
>          "(%ld > %ld)\n", ber->ber_len, sb->sb_max_incoming );
>      #endif
>      errno = ERANGE;
>      return LBER_DEFAULT;
>    }
>
>    /* allocate data buffer */
>    {
>      char*     buffer;
>      ber_len_t left_over;
>
>      buffer = LBER_MALLOC(ber->ber_len + 1);
>      if (buffer == NULL)
>        return LBER_DEFAULT;
>
>      /* copy left over bytes from temp to data buffer */
>      left_over = ber->ber_end - ber->ber_rwptr;
>      if (left_over > 0)
>        AC_MEMCPY(buffer, ber->ber_rwptr, left_over);
>
>      ber->ber_buf   = buffer;
>      ber->ber_ptr   = buffer;
>      ber->ber_end   = buffer + ber->ber_len;
>      ber->ber_rwptr = buffer + left_over;
>    }
>  } /* parse the tag and length */
>
>  /* read the data */
>  if (ber->ber_buf <= ber->ber_rwptr && ber->ber_rwptr < ber->ber_end) {
>    ber_len_t sblen = ber_int_sb_read(sb, ber->ber_rwptr,
>                        ber->ber_end - ber->ber_rwptr);
>    if (sblen <= 0)
>      return LBER_DEFAULT;
>
>    ber->ber_rwptr += sblen;
>
>    /* more data */
>    if (ber->ber_rwptr < ber->ber_end) {
>      #if defined(EWOULDBLOCK)
>        errno = EWOULDBLOCK;
>      #elif defined(EAGAIN)
>        errno = EAGAIN;
>      #endif
>      return LBER_DEFAULT;
>    }
>
>    /* the whole element is read if we reach this point */
>    ber->ber_rwptr = NULL;
>    *len           = ber->ber_len;
>
>    /* dump if debugging is switched on */
>    if (ber->ber_debug) {
>      #ifdef NEW_LOGGING
>        LDAP_LOG( BER, DETAIL1,
>          "ber_get_next: tag 0x%lx len %ld\n",
>          ber->ber_tag, ber->ber_len, 0  );
>        if (LDAP_LOGS_TEST(BER, DETAIL2))
>          BER_DUMP(( "liblber", LDAP_LEVEL_DETAIL2, ber, 1 ));
>      #else
>        ber_log_printf( LDAP_DEBUG_TRACE, ber->ber_debug,
>          "ber_get_next: tag 0x%lx len %ld contents:\n",
>          ber->ber_tag, ber->ber_len );
>        ber_log_dump( LDAP_DEBUG_BER, ber->ber_debug, ber, 1 );
>      #endif
>    }
>
>    return ber->ber_tag;
>  }
>
>  /* the ber structure is messed up if we reach this point */
>  assert( 0 );
>  return LBER_DEFAULT;
>} /* ber_get_next */
References:
- FW: ldapsearch problems (ITS#2490)
  - From: "Howard Chu" <hyc@highlandsun.com>
Prev by Date: Re: EBCDIC slapd
Next by Date: HEADS UP: TLS "hard"
Index(es):
- Chronological
- Thread