outstanding telnetd bug fix, and two new features

[Denys Vlasenko]

On Wednesday 12 November 2008 20:35, Cathey, Jim wrote:
> Both approaches seem to work just fine.  The add_iacs()
> patch I did is smaller, and I believe fits better into
> the existing structure of telnetd, such as it is:
> 
> $ size networking/telnetd.o
>    text    data     bss     dec     hex filename
>    3752      16      16    3784     ec8 networking/telnetd.o
> $ # Replace add_iacs() patch with iac_safe_write() patch & rebuild...
> $ size networking/telnetd.o
>    text    data     bss     dec     hex filename
>    3792      16      16    3824     ef0 networking/telnetd.o
> $ 
> 
> (These are mips64 numbers.)
> 
> I had to apply iac_safe_write() code as a patch to what we
> had, replacing my patch.  (For comparison purposes.)  I couldn't
> get the current svn to fully build for me, and I've got nearly
> zero time to spend on this.  I believe the comparative results
> are still valid, however.
> 
> Essentially both patches each apply one new subroutine, which
> are listed here for easy comparison:
> 
> static void
> add_iacs(struct tsession *ts, int *pnum_fromtty)
> {
> 	unsigned char *ptr = TS_BUF2 + ts->rdidx2;
> 	unsigned char *end = ptr + MIN(BUFSIZE - ts->rdidx2, *pnum_fromtty);

Let's assume there are full "BUFSIZE - ts->rdidx2" bytes
in the buffer, and the very first byte is 0xff.

> 	while (ptr < end) {
> 		if (*ptr == IAC) {
> 			memmove(ptr+1, ptr, end - ptr);// Results in dupe IAC!

Here we would store one byte past the buffer's end.

> 			if (end < TS_BUF2 + BUFSIZE)
> 				end++;
...else - just forget that pesky last byte, who cares?

> 			(*pnum_fromtty)++;
> 			ptr++; // Don't process the dupe next time either.
> 		}
> 		ptr++;
> 	}
> }
> 
> *** Versus ***
> 
> static size_t iac_safe_write(int fd, const char *buf, size_t count)
> {
> 	const char *oxff;
> 	size_t wr, rc, total;
> 
> 	total = 0;
> 	while (1) {
> 		if (count == 0)
> 			return total;
> 		if (*buf == (char)0xff) {
> 			rc = safe_write(fd, "\xff\xff", 2);
> 			if (rc != 2)
> 				break;
> 			buf++;
> 			total++;
> 			count--;
> 			continue;
> 		}
> 		/* count != 0, *buf != 0xff */
> 		oxff = memchr(buf, 0xff, count);
> 		wr = count;
> 		if (oxff)
> 			wr = oxff - buf;
> 		rc = safe_write(fd, buf, wr);
> 		if (rc != wr)
> 			break;
> 		buf += rc;
> 		total += rc;
> 		count -= rc;
> 	}
> 	/* here: rc - result of last short write */
> 	if ((ssize_t)rc < 0) { /* error? */
> 		if (total == 0)
> 			return rc;
> 		rc = 0;
> 	}
> 	return total + rc;
> }
> 
> I happen to think that mine is simpler; it's for certain smaller!

I am all for smaller, as long as it does not introduce bugs.
--
vda