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

[ellert@tsl.uu.se]

On Wed, 27 Nov 2002, Howard Chu wrote:

> I see what you're getting at, but I don't see why there is still a problem.
> If ber_pvt_sb_do_write doesn't write the entire buffer in the first call, it
> will try to flush in a subsequent call. There will always be a subsequent
> call, because eventually the search result code must be written out. The code
> in 2.1.8 has already been tested successfully with very large search entries.

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. 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;
}

In version 2.0.22 the ber_pvt_sb_do_write function had the following
lines (close to the end):

	if ( (ber_len_t)ret < to_go ) {
		/* not enough data, stend no data was sent. */
		return -1;
	}

which meant that if the full buffer was not written it was reported as a
return value of -1. These lines are not there any more (neither in 2.0.27,
nor in 2.1.8, nor in head). This means that this check has to be done in
sb_sasl_write instead (or be put back into ber_pvt_sb_do_write, but there
might be other functions calling ber_pvt_sb_do_write that don't like this
-- it was removed for some reason). 

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. 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. 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