Hello!
I have a minor observation about code in sshbuf.c, not sure if it would be
useful, but here it is.
sshbuf_reset() is currently implemented like this:
void
sshbuf_reset(struct sshbuf *buf)
{
u_char *d;
if (buf->readonly || buf->refcount > 1) {
/* Nonsensical. Just make buffer appear empty */
buf->off = buf->size;
return;
}
if (sshbuf_check_sanity(buf) != 0)
return;
buf->off = buf->size = 0;
if (buf->alloc != SSHBUF_SIZE_INIT) {
if ((d = recallocarray(buf->d, buf->alloc, SSHBUF_SIZE_INIT,
1)) != NULL) {
buf->cd = buf->d = d;
buf->alloc = SSHBUF_SIZE_INIT;
}
}
explicit_bzero(buf->d, buf->alloc);
}
This function allocates a new buffer of size SSHBUF_SIZE_INIT if
buf->alloc != SSHBUF_SIZE_INIT, which can put buf in an inconsistent
state if buf->max_size < SSHBUF_SIZE_INIT, because it will make
buf->alloc > buf->max_size true, which will trigger an error with a
next call to sshbuf_check_sanity(). For example, struct sshbuf *buf =
sshbuf_new(); sshbuf_set_max_size(buf, 100); sshbuf_reset(buf); will
lead to SSH_ERR_INTERNAL_ERROR. This code is of course just for
demonstration, but the thing is that an sshbuf object can be put into
invalid state through its public API. Or it is just assumed that no
one will ever set ->max_size to a value less than SSHBUF_SIZE_INIT?
Anyway, i thought that all invariants of sshbuf object must be
preserved by its own API no matter how stupid the use of this API is,
so i wrote this.
Also, why a call to sshbuf_check_sanity() in sshbuf_reset() is made
after dereferencing a pointer which is potentialy a NULL pointer? I
think a call to sshbuf_check_sanity() should precede other operations.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
