RE: sb_sasl_write doesn't handle partial writes (ITS#2211)

[hyc@highlandsun.com]

> -----Original Message-----
> From: ellert@tsl.uu.se [mailto:ellert@tsl.uu.se]

> The first call to ber_pvt_sb_do_write is really the one close
> to the end
> of the function:
>
> 	ret = ber_pvt_sb_do_write( sbiod, &p->buf_out );
> 	if ( ret <= 0 )
> 		return ret;
> 	return len;
> }
>
> So if there is an error (i.e. if ret <= 0) it will be
> returned, otherwise
> the function will return len, which means that the whole
> buffer has been
> sent, which is not true for a partial write.

The whole buffer has not been written, but "len" bytes were encoded. On a
subsequent call, the caller must advance their buffer pointer by len, to
avoid re-encoding the same data.

> You therefore
> have to check
> for this:
>
> 	ret = ber_pvt_sb_do_write( sbiod, &p->buf_out );
> 	if ( ret <= 0 )
> 		return ret;
> 	if ( p->buf_out.buf_ptr != p->buf_out.buf_end )
> 		return 0;
> 	return len;
> }

No. If you return 0 here, then the caller will not advance its output
pointer, and will retry with the same output buffer. That is wrong.

> Then looking at what happens at the recall:
>
> 	/* Are there anything left in the buffer? */
> 	if ( p->buf_out.buf_ptr != p->buf_out.buf_end ) {
> 		ret = ber_pvt_sb_do_write( sbiod, &p->buf_out );
> 		if ( ret < 0 )
> 			return ret;
> 		/* Still have something left?? */
> 		if ( p->buf_out.buf_ptr != p->buf_out.buf_end ) {
> 			errno = EAGAIN;
> 			return 0;
> 		}
> 	}
>
> Here the check for p->buf_out.buf_ptr != p->buf_out.buf_end after the
> comment /* Still have ... */ has been put in between version
> 2.0.22 and
> version 2.0.27 (replacing the check removed from the
> ber_pvt_sb_do_write
> function).
>
> The sb_sasl_write will be called again and again until the
> full buffer is
> written. The problem here is what happens when the buffer is finally
> written and p->buf_out.buf_ptr == p->buf_out.buf_end after
> the return from
> the call to ber_pvt_sb_do_write. The correct thing to do here
> is to return
> len, indicating that the full buffer is written. Instead, the
> code just
> continues and starts to SASL encode the same buffer it just
> successfully
> sent.

No. The full buffer being checked here is the remainder of the previous
write. When it gets down to the SASL encode step, it is encoding the new
buffer from the current call.

> With my patch:
>
> 	/* Is there anything left in the buffer? */
> 	if ( p->buf_out.buf_ptr != p->buf_out.buf_end ) {
> 		ret = ber_pvt_sb_do_write( sbiod, &p->buf_out );
> 		if ( ret <= 0 )
> 			return ret;
> 		if ( p->buf_out.buf_ptr != p->buf_out.buf_end )
> 			return 0;
> 		return len;
> 	}
>
> The most important thing here is the added 'return len'
> statement.

No. This is the len from the current call and current buffer, but this code
was flushing the previous buffer. This "len" is the wrong value to return at
this point, because no data from the current write request has been prcoessed
yet.

> Also
> Note that the lines of code inside the if statement above is
> exactly the
> same as the code at the end of the function (since you really
> do want to
> handle a partially written buffer in the same way after the
> first try to
> write it and after the second, third, etc. try to write it).
>
> Since the patched code needs the value of len earlier I also moved the
> check for len exceeding the size of the SASL buffer to the begining of
> the function.
>
> 	Mattias

Have you actually run the 2.1.8 code and encountered any data loss? As I've
said, the code has been tested with large entries and is known to work. Your
analysis and patch are incorrect.

  -- Howard Chu
  Chief Architect, Symas Corp.       Director, Highland Sun
  http://www.symas.com               http://highlandsun.com/hyc
  Symas: Premier OpenSource Development and Support