Skip to content

Commit

Permalink
socket.c: Don't cast ssize_t to size_t.
Browse files Browse the repository at this point in the history
bbs_node_readline improperly casted a ssize_t
to size_t, which meant that anytime -1 was
returned, this overflowed to a really large
number, causing memchr to read until it
triggered a segmentation fault.

This is fixed by not casting to size_t here,
and relevant functions have been updated to
return ssize_t instead of int, to make the
return values more semantic.

Fixes #23
  • Loading branch information
InterLinked1 committed Oct 21, 2023
1 parent 600bddb commit 7d9dfbf
Show file tree
Hide file tree
Showing 9 changed files with 51 additions and 49 deletions.
4 changes: 2 additions & 2 deletions bbs/editor.c
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ int bbs_pager(struct bbs_node *node, struct pager_info *pginfo, int ms, const ch

/* Actual paging is now required. */
for (;;) {
int res;
ssize_t res;
char buf[5];
NEG_RETURN(bbs_node_writef(node, s ? ":" : "EOF:")); /* Input prompt */
res = bbs_node_poll_read(node, ms, buf, 5); /* Any key shouldn't be more than 5 characters (some keys involve escape sequences) */
Expand All @@ -221,7 +221,7 @@ int bbs_pager(struct bbs_node *node, struct pager_info *pginfo, int ms, const ch
*/
}
if (res < 0) {
return res;
return -1;
} else if (!res) {
return 1; /* Timeout */
}
Expand Down
21 changes: 11 additions & 10 deletions bbs/readline.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ void bbs_readline_flush(struct readline_data *rldata)
rldata->leftover = 0;
}

static char *readline_pre_read(struct readline_data *restrict rldata, const char *delim, int *resptr)
static char *readline_pre_read(struct readline_data *restrict rldata, const char *delim, ssize_t *resptr)
{
char *firstdelim = NULL;

Expand All @@ -73,7 +73,7 @@ static char *readline_pre_read(struct readline_data *restrict rldata, const char
firstdelim = memmem(rldata->buf, res, delim, strlen(delim)); /* Use buf, not pos, since pos is the beginning of the buffer that remains at this point. */
res = rldata->leftover = 0;
rldata->leftover = 0;
*resptr = (int) res;
*resptr = (ssize_t) res;
} else {
if (!rldata->waiting) {
/* bbs_readline never returns without reading a full line,
Expand All @@ -99,7 +99,7 @@ int readline_bytes_available(struct readline_data *restrict rldata, int process)
return (int) (rldata->pos - rldata->buf);
}

static int readline_post_read(struct readline_data *restrict rldata, const char *delim, char *restrict firstdelim, int res)
static int readline_post_read(struct readline_data *restrict rldata, const char *delim, char *restrict firstdelim, ssize_t res)
{
int used, delimlen;

Expand All @@ -111,7 +111,7 @@ static int readline_post_read(struct readline_data *restrict rldata, const char
firstdelim += delimlen; /* There is the beginning of the rest of the buffer. No, we do not need to add 1 here. */
rldata->leftover = (size_t) (rldata->pos - firstdelim); /* Number of bytes leftover. */
#ifdef EXTRA_DEBUG
bbs_debug(8, "Read %lu bytes (%d just now), processing %d and leaving %lu leftover\n", rldata->pos - rldata->buf, res, used, rldata->leftover);
bbs_debug(8, "Read %lu bytes (%ld just now), processing %d and leaving %lu leftover\n", rldata->pos - rldata->buf, res, used, rldata->leftover);
#else
UNUSED(res);
#endif
Expand All @@ -124,7 +124,7 @@ static int readline_post_read(struct readline_data *restrict rldata, const char
/*! \brief Helper function to read a single line from a file descriptor, with a timeout (for any single read) */
ssize_t bbs_readline(int fd, struct readline_data *restrict rldata, const char *restrict delim, int timeout)
{
int res;
ssize_t res;
char *firstdelim;

firstdelim = readline_pre_read(rldata, delim, &res);
Expand All @@ -139,7 +139,7 @@ ssize_t bbs_readline(int fd, struct readline_data *restrict rldata, const char *
}
res = bbs_poll_read(fd, timeout, rldata->pos, (size_t) rldata->left - 1); /* Subtract 1 for NUL */
if (res <= 0) {
bbs_debug(3, "bbs_poll_read returned %d\n", res);
bbs_debug(3, "bbs_poll_read returned %ld\n", res);
return res - 1; /* see the doxygen notes: we should return 0 only if we read just the delimiter. */
}
rldata->pos[res] = '\0'; /* Safe. Null terminate so we can use string functions. */
Expand All @@ -155,7 +155,7 @@ ssize_t bbs_readline(int fd, struct readline_data *restrict rldata, const char *
static ssize_t __bbs_readline_getn(int fd, int destfd, struct dyn_str *restrict dynstr, struct readline_data *restrict rldata, int timeout, size_t n)
{
ssize_t wres;
int res;
ssize_t res;
size_t left_in_buffer, written = 0, remaining = n;

/* First, use anything that's already in the buffer from a previous read.
Expand Down Expand Up @@ -333,7 +333,7 @@ static int readline_get_until_process(struct dyn_str *dynstr, struct readline_da

int bbs_readline_get_until(int fd, struct dyn_str *dynstr, struct readline_data *restrict rldata, int timeout, size_t maxlen)
{
int res;
ssize_t res;
size_t left_in_buffer;

bbs_assert_exists(rldata->boundary); /* Boundary must be initialized first */
Expand All @@ -354,7 +354,7 @@ int bbs_readline_get_until(int fd, struct dyn_str *dynstr, struct readline_data
bbs_assert(rldata->pos == rldata->buf);
res = bbs_poll_read(fd, timeout, rldata->pos, rldata->left);
if (res <= 0) {
return res;
return -1;
}
rldata->pos += (size_t) res;
rldata->left -= (size_t) res;
Expand All @@ -370,7 +370,8 @@ int bbs_readline_append(struct readline_data *restrict rldata, const char *restr
{
char *firstdelim;
size_t res;
int unused, drain = 0;
ssize_t unused;
int drain = 0;

firstdelim = readline_pre_read(rldata, delim, &unused);
if (firstdelim) {
Expand Down
42 changes: 21 additions & 21 deletions bbs/socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -1490,50 +1490,50 @@ int bbs_node_tpoll(struct bbs_node *node, int ms)
return res;
}

int bbs_node_read(struct bbs_node *node, char *buf, size_t len)
ssize_t bbs_node_read(struct bbs_node *node, char *buf, size_t len)
{
int res;
ssize_t res;

REQUIRE_SLAVE_FD(node);

bbs_node_lock(node);
res = (int) read(node->slavefd, buf, len);
res = read(node->slavefd, buf, len);
bbs_node_unlock(node);
if (res <= 0) {
bbs_debug(5, "Node %d: read returned %d\n", node->id, res);
bbs_debug(5, "Node %d: read returned %ld\n", node->id, res);
}

if (res == 1) {
bbs_debug(10, "Node %d: read %d byte (%d)\n", node->id, res, *buf);
bbs_debug(10, "Node %d: read %ld byte (%d)\n", node->id, res, *buf);
} else {
bbs_debug(10, "Node %d: read %d bytes\n", node->id, res);
bbs_debug(10, "Node %d: read %ld bytes\n", node->id, res);
}
return res;
}

int bbs_node_poll_read(struct bbs_node *node, int ms, char *buf, size_t len)
ssize_t bbs_node_poll_read(struct bbs_node *node, int ms, char *buf, size_t len)
{
int res = bbs_node_poll(node, ms);
ssize_t res = bbs_node_poll(node, ms);
if (res <= 0) {
return res;
}
res = bbs_node_read(node, buf, len);
return res;
}

int bbs_poll_read(int fd, int ms, char *buf, size_t len)
ssize_t bbs_poll_read(int fd, int ms, char *buf, size_t len)
{
int res = bbs_poll(fd, ms);
ssize_t res = bbs_poll(fd, ms);
if (res <= 0) {
return res;
}
res = (int) read(fd, buf, len);
res = read(fd, buf, len);
return res;
}

int bbs_expect(int fd, int ms, char *buf, size_t len, const char *str)
{
int res;
ssize_t res;

*buf = '\0'; /* Clear the buffer, in case we don't read anything at all. */
res = bbs_poll_read(fd, ms, buf, len - 1);
Expand Down Expand Up @@ -1601,7 +1601,7 @@ char bbs_node_tread(struct bbs_node *node, int ms)

if (res > 0) {
char buf[1];
res = (char) bbs_node_read(node, buf, sizeof(buf));
res = (signed char) bbs_node_read(node, buf, sizeof(buf));
if (res <= 0) {
return res;
}
Expand Down Expand Up @@ -1735,7 +1735,7 @@ int bbs_node_readline(struct bbs_node *node, int ms, char *buf, size_t len)
}

for (;;) {
size_t bytes;
ssize_t bytes;
if (keep_trying) {
res = bbs_node_poll(node, 5);
if (res == 0) {
Expand All @@ -1751,12 +1751,12 @@ int bbs_node_readline(struct bbs_node *node, int ms, char *buf, size_t len)
bbs_debug(10, "Node %d: poll returned %d\n", node->id, res);
return res;
}
bytes = (size_t) bbs_node_read(node, buf, len);
bytes = bbs_node_read(node, buf, len);
if (bytes <= 0) {
bbs_debug(10, "Node %d: read returned %ld\n", node->id, bytes);
return (int) res;
return (int) bytes;
}
nterm = memchr(buf, '\0', bytes);
nterm = memchr(buf, '\0', (size_t) bytes);
/* Telnet may send CR NUL or CR LF, so check CR first, then LF.
* To make things even more confusing, Windows Telnet and SyncTERM seem to send LF LF.
* (Though it could be the PTY line discipline converting things that results in this)
Expand All @@ -1769,13 +1769,13 @@ int bbs_node_readline(struct bbs_node *node, int ms, char *buf, size_t len)
* In PuTTY with Telnet, there are ^@'s at the beginning of lines after this function returns.
*/
if (!term) { /* In the case where we poll again for a few ms (below), this will be true, don't do this again. */
term = memchr(buf, '\r', bytes); /* There is no strnchr function. Use memchr. */
term = memchr(buf, '\r', (size_t) bytes); /* There is no strnchr function. Use memchr. */
if (!term) {
term = memchr(buf, '\n', bytes);
term = memchr(buf, '\n', (size_t) bytes);
}
}
buf += bytes;
left -= bytes;
left -= (size_t) bytes;
bytes_read += (int) bytes;
#ifdef DEBUG_TEXT_IO
for (i = 0; i < bytes_read; i++) {
Expand Down Expand Up @@ -1944,7 +1944,7 @@ int bbs_node_flush_input(struct bbs_node *node)
if (res <= 0) {
break;
}
res = bbs_node_read(node, buf, sizeof(buf));
res = (int) bbs_node_read(node, buf, sizeof(buf));
if (res <= 0) {
break;
}
Expand Down
6 changes: 3 additions & 3 deletions doors/door_chat.c
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ static int chat_run(struct bbs_node *node, struct participant *p)
{
char buf[384];
char buf2[sizeof(buf)];
int res;
ssize_t res;
int participants;
struct channel *c = p->channel;

Expand Down Expand Up @@ -396,7 +396,7 @@ static int chat_run(struct bbs_node *node, struct participant *p)
* Even if it doesn't, don't blindly add one. Because we write the time and message in 2 separate calls to write(),
* in __chat_send, we could end up doing 2 disjoint reads here, and the first one will only have the timestamp.
*/
if (bbs_node_writef(node, "%.*s", res, buf) < 0) {
if (bbs_node_writef(node, "%.*s", (int) res, buf) < 0) {
res = -1;
break;
}
Expand All @@ -414,7 +414,7 @@ static int chat_run(struct bbs_node *node, struct participant *p)
}

chat_send(c, NULL, "%s has left #%s from node %d\n", bbs_username(node->user), c->name, node->id);
return res;
return (int) res;
}

static int module_shutdown = 0;
Expand Down
8 changes: 4 additions & 4 deletions doors/door_irc.c
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ static int participant_relay(struct bbs_node *node, struct participant *p, const
{
char buf[384];
char buf2[sizeof(buf)];
int res;
ssize_t res;
struct client_relay *c = p->client;

/* Join the channel */
Expand Down Expand Up @@ -276,7 +276,7 @@ static int participant_relay(struct bbs_node *node, struct participant *p, const
bbs_node_buffer(node);
res = bbs_node_poll_read(node, MIN_MS(3), buf + 1, sizeof(buf) - 2); /* Leave the first char in the buffer alone, -1 for null termination, and -1 for the first char */
if (res <= 0) {
bbs_debug(3, "bbs_node_poll_read returned %d\n", res);
bbs_debug(3, "bbs_node_poll_read returned %ld\n", res);
if (res == 0) {
/* User started a message, but didn't finish before timeout */
bbs_node_writef(node, "\n*** TIMEOUT ***\n");
Expand Down Expand Up @@ -305,7 +305,7 @@ static int participant_relay(struct bbs_node *node, struct participant *p, const
}
buf[res] = '\0'; /* Safe */
/* Don't add a trailing LF, the sent message should already had one. */
if (bbs_node_writef(node, "%.*s", res, buf) < 0) {
if (bbs_node_writef(node, "%.*s", (int) res, buf) < 0) {
res = -1;
break;
}
Expand All @@ -323,7 +323,7 @@ static int participant_relay(struct bbs_node *node, struct participant *p, const
}

chat_send(c, NULL, channel, "%s@%d has left %s\n", bbs_username(node->user), node->id, channel);
return res;
return (int) res;
}

/*! \note Must be called locked */
Expand Down
6 changes: 3 additions & 3 deletions include/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ int bbs_node_tpoll(struct bbs_node *node, int ms);
* \param len Size of buf
* \retval Same as read() i.e. 0 if fd closed, -1 on failure, positive number of bytes read otherwise
*/
int bbs_node_read(struct bbs_node *node, char *buf, size_t len);
ssize_t bbs_node_read(struct bbs_node *node, char *buf, size_t len);

/*!
* \brief wrapper around poll() and read() for BBS node
Expand All @@ -376,7 +376,7 @@ int bbs_node_read(struct bbs_node *node, char *buf, size_t len);
* \param len Size of buf
* \retval Same as poll() and read()
*/
int bbs_node_poll_read(struct bbs_node *node, int ms, char *buf, size_t len);
ssize_t bbs_node_poll_read(struct bbs_node *node, int ms, char *buf, size_t len);

/*!
* \brief wrapper around poll() and read()
Expand All @@ -386,7 +386,7 @@ int bbs_node_poll_read(struct bbs_node *node, int ms, char *buf, size_t len);
* \param len Size of buf
* \retval Same as poll() and read()
*/
int bbs_poll_read(int fd, int ms, char *buf, size_t len);
ssize_t bbs_poll_read(int fd, int ms, char *buf, size_t len);

/*!
* \brief wrapper around bbs_poll_read that expects a substring to appear in the read response
Expand Down
2 changes: 1 addition & 1 deletion nets/net_finger.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static void *finger_handler(void *varg)
char *username = NULL;
char *hostname = NULL;
int verbose = 0;
int res;
ssize_t res;

/* This thread is running instead of the normal node handler thread */
/* Remember, no pseudoterminal is allocated for this node! Can NOT use normal bbs_ I/O functions. */
Expand Down
4 changes: 2 additions & 2 deletions nets/net_gopher.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ static void *gopher_handler(void *varg)
struct bbs_node *node = varg;
char *tmp;
struct stat st;
int res;
ssize_t res;

/* This thread is running instead of the normal node handler thread */
/* Remember, no pseudoterminal is allocated for this node! Can NOT use normal bbs_ I/O functions. */
Expand Down Expand Up @@ -109,7 +109,7 @@ static void *gopher_handler(void *varg)

res = (int) sendfile(node->fd, fileno(fp), &offset, (size_t) size); /* We must manually tell it the offset or it will be at the EOF, even with rewind() */
if (res != size) {
bbs_error("sendfile failed (%d): %s\n", res, strerror(errno));
bbs_error("sendfile failed (%ld): %s\n", res, strerror(errno));
}
fclose(fp);
} else {
Expand Down
7 changes: 4 additions & 3 deletions nets/net_rlogin.c
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ static int send_urgent(int fd)

static int rlogin_handshake(struct bbs_node *node)
{
int i, res;
int i;
ssize_t res;
char buf[128];
unsigned char buf2[128];
int on = 1;
Expand All @@ -85,14 +86,14 @@ static int rlogin_handshake(struct bbs_node *node)
buf[res] = '\0'; /* Safe - just in case we didn't read a NUL */
i = bbs_strncount(buf, (size_t) res, '\0');
if (i != 4) {
bbs_debug(3, "Got %d-byte connection string with %d NULs?\n", res, i);
bbs_debug(3, "Got %ld-byte connection string with %d NULs?\n", res, i);
return -1;
}
s1 = buf;
s2 = s1 + strlen(s1) + 1;
s3 = s2 + strlen(s2) + 1;
s4 = s3 + strlen(s3) + 1;
bbs_debug(3, "Got %d-byte connection string (%s/%s/%s/%s)\n", res, s1, s2, s3, s4);
bbs_debug(3, "Got %ld-byte connection string (%s/%s/%s/%s)\n", res, s1, s2, s3, s4);
if (SWRITE(node->fd, "\0") != STRLEN("\0")) { /* Send 0-byte to ACK and change to data transfer mode */
return -1;
}
Expand Down

0 comments on commit 7d9dfbf

Please sign in to comment.