Skip to content

Commit a27b989

Browse files
millertmillert
committed
Check for negative return value of read, write and lseek instead of -1
The return values are used in ways that assume they are positive. In practice, it is not possible to have a negative return value other than -1 due to the size of the buffers being read from or written to. Also add overflow checks when updating the buffer len. Quiets several coverity warnings.
1 parent 6df9678 commit a27b989

22 files changed

Lines changed: 172 additions & 124 deletions

‎lib/fuzzstub/fuzzstub.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ main(int argc, char *argv[])
104104
}
105105
nread = read(fd, buf, filesize);
106106
if ((size_t)nread != filesize) {
107-
if (nread == -1)
107+
if (nread < 0)
108108
fprintf(stderr, "read %s: %s\n", arg, strerror(errno));
109109
else
110110
fprintf(stderr, "read %s: short read\n", arg);

‎lib/iolog/iolog_nextid.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ iolog_nextid(const char *iolog_dir, char sessid[7])
107107
/* Read current seq number (base 36). */
108108
nread = read(fd, buf, sizeof(buf) - 1);
109109
if (nread != 0) {
110-
if (nread == -1) {
110+
if (nread < 0) {
111111
goto done;
112112
}
113113
if (buf[nread - 1] == '\n')

‎lib/iolog/regress/iolog_filter/check_iolog_filter.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ main(int argc, char *argv[])
138138

139139
nread = read(fd, tbuf, timing.u.nbytes);
140140
if ((size_t)nread != timing.u.nbytes) {
141-
if (nread == -1)
141+
if (nread < 0)
142142
sudo_warn("%s/%s", argv[i], name);
143143
else
144144
sudo_warnx("%s/%s: short read", argv[i], name);
@@ -155,8 +155,8 @@ main(int argc, char *argv[])
155155

156156
if (timing.event == IO_EVENT_TTYIN) {
157157
nread = read(ttyin_ok_fd, fbuf, timing.u.nbytes);
158-
if (nread == -1) {
159-
if (nread == -1)
158+
if ((size_t)nread != timing.u.nbytes) {
159+
if (nread < 0)
160160
sudo_warn("%s/ttyin.filtered", argv[i]);
161161
else
162162
sudo_warnx("%s/ttyin.filtered: short read", argv[i]);

‎lib/util/event.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ signal_pipe_cb(int fd, int what, void *v)
154154
sudo_debug_printf(SUDO_DEBUG_INFO,
155155
"%s: received signal %d", __func__, (int)ch);
156156
}
157-
if (nread == -1 && errno != EAGAIN) {
157+
if (nread < 0 && errno != EAGAIN) {
158158
sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_LINENO|SUDO_DEBUG_ERRNO,
159159
"%s: error reading from signal pipe fd %d", __func__, fd);
160160
}

‎lib/util/regress/fuzz/fuzz_sudo_conf.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
8585
if (fd == -1)
8686
return 0;
8787
nwritten = write(fd, data, size);
88-
if (nwritten == -1) {
88+
if ((size_t)nwritten != size) {
8989
close(fd);
9090
return 0;
9191
}

‎logsrvd/logsrvd.c‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -969,13 +969,17 @@ server_msg_cb(int fd, int what, void *v)
969969
} else
970970
#endif
971971
{
972-
nwritten = (size_t)write(fd, buf->data + buf->off, buf->len - buf->off);
972+
const ssize_t n = write(fd, buf->data + buf->off, buf->len - buf->off);
973+
if (n < 0) {
974+
if (errno == EAGAIN || errno == EINTR)
975+
debug_return;
976+
sudo_warn("%s: write", closure->ipaddr);
977+
goto finished;
978+
}
979+
nwritten = (size_t)n;
973980
}
974-
975-
if (nwritten == (size_t)-1) {
976-
if (errno == EAGAIN || errno == EINTR)
977-
debug_return;
978-
sudo_warn("%s: write", closure->ipaddr);
981+
if (nwritten > SIZE_MAX - buf->off) {
982+
sudo_warnx(U_("internal error, %s overflow"), __func__);
979983
goto finished;
980984
}
981985
buf->off += nwritten;
@@ -1082,25 +1086,28 @@ client_msg_cb(int fd, int what, void *v)
10821086
} else
10831087
#endif
10841088
{
1085-
nread = (size_t)read(fd, buf->data + buf->len, buf->size - buf->len);
1089+
const ssize_t n = read(fd, buf->data + buf->len, buf->size - buf->len);
1090+
if (n < 0) {
1091+
if (errno == EAGAIN || errno == EINTR)
1092+
debug_return;
1093+
sudo_warn("%s: read", closure->ipaddr);
1094+
goto close_connection;
1095+
}
1096+
nread = (size_t)n;
10861097
}
10871098

10881099
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: received %zd bytes from client %s",
10891100
__func__, nread, closure->ipaddr);
1090-
switch (nread) {
1091-
case (size_t)-1:
1092-
if (errno == EAGAIN || errno == EINTR)
1093-
debug_return;
1094-
sudo_warn("%s: read", closure->ipaddr);
1095-
goto close_connection;
1096-
case 0:
1101+
if (nread == 0) {
10971102
if (closure->state != FINISHED) {
10981103
sudo_debug_printf(SUDO_DEBUG_WARN|SUDO_DEBUG_LINENO,
10991104
"unexpected EOF");
11001105
}
11011106
goto close_connection;
1102-
default:
1103-
break;
1107+
}
1108+
if (nread > SIZE_MAX - buf->len) {
1109+
sudo_warnx(U_("internal error, %s overflow"), __func__);
1110+
goto close_connection;
11041111
}
11051112
buf->len += nread;
11061113

‎logsrvd/logsrvd_local.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,7 @@ store_exit_info_json(int dfd, struct eventlog *evlog)
376376
iov[1].iov_len = sudo_json_get_len(&jsonc);
377377
iov[2].iov_base = (char *)"\n}\n";
378378
iov[2].iov_len = 3;
379-
if (writev(fd, iov, 3) == -1) {
379+
if (writev(fd, iov, 3) < 0) {
380380
sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_LINENO|SUDO_DEBUG_ERRNO,
381381
"unable to write %s/log.json", evlog->iolog_path);
382382
/* Back up and try to restore to original state. */

‎logsrvd/logsrvd_relay.c‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -798,23 +798,26 @@ relay_server_msg_cb(int fd, int what, void *v)
798798
} else
799799
#endif
800800
{
801+
ssize_t n;
802+
801803
sudo_debug_printf(SUDO_DEBUG_INFO,
802804
"%s: ServerMessage from relay %s (%s)", __func__,
803805
relay_closure->relay_name.name, relay_closure->relay_name.ipaddr);
804-
nread = (size_t)read(fd, buf->data + buf->len, buf->size - buf->len);
806+
n = read(fd, buf->data + buf->len, buf->size - buf->len);
807+
if (n < 0) {
808+
if (errno == EAGAIN || errno == EINTR)
809+
debug_return;
810+
sudo_warn("%s: read", relay_closure->relay_name.ipaddr);
811+
closure->errstr = _("error reading from relay");
812+
goto send_error;
813+
}
814+
nread = (size_t)n;
805815
}
806816

807817
sudo_debug_printf(SUDO_DEBUG_INFO,
808818
"%s: received %zd bytes from relay %s (%s)", __func__, nread,
809819
relay_closure->relay_name.name, relay_closure->relay_name.ipaddr);
810-
switch (nread) {
811-
case (size_t)-1:
812-
if (errno == EAGAIN || errno == EINTR)
813-
debug_return;
814-
sudo_warn("%s: read", relay_closure->relay_name.ipaddr);
815-
closure->errstr = _("unable to read from relay");
816-
goto send_error;
817-
case 0:
820+
if (nread == 0) {
818821
/* EOF from relay server, close the socket. */
819822
shutdown(relay_closure->sock, SHUT_RDWR);
820823
close(relay_closure->sock);
@@ -833,8 +836,11 @@ relay_server_msg_cb(int fd, int what, void *v)
833836
if (closure->sock == -1)
834837
connection_close(closure);
835838
debug_return;
836-
default:
837-
break;
839+
}
840+
if (nread > SIZE_MAX - buf->len) {
841+
sudo_warnx(U_("internal error, %s overflow"), __func__);
842+
closure->errstr = _("error reading from relay");
843+
goto send_error;
838844
}
839845
buf->len += nread;
840846

@@ -979,14 +985,20 @@ relay_client_msg_cb(int fd, int what, void *v)
979985
} else
980986
#endif
981987
{
982-
nwritten = (size_t)write(fd, buf->data + buf->off, buf->len - buf->off);
983-
if (nwritten == (size_t)-1) {
988+
const ssize_t n = write(fd, buf->data + buf->off, buf->len - buf->off);
989+
if (n < 0) {
984990
if (errno == EAGAIN || errno == EINTR)
985991
debug_return;
986992
sudo_warn("%s: write", relay_closure->relay_name.ipaddr);
987993
closure->errstr = _("error writing to relay");
988994
goto send_error;
989995
}
996+
nwritten = (size_t)n;
997+
}
998+
if (nwritten > SIZE_MAX - buf->off) {
999+
sudo_warnx(U_("internal error, %s overflow"), __func__);
1000+
closure->errstr = _("error writing to relay");
1001+
goto send_error;
9901002
}
9911003
buf->off += nwritten;
9921004

‎logsrvd/regress/fuzz/fuzz_logsrvd_conf.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
193193
if (fd == -1)
194194
return 0;
195195
nwritten = write(fd, data, size);
196-
if (nwritten == -1) {
196+
if ((size_t)nwritten != size) {
197197
close(fd);
198198
return 0;
199199
}

‎logsrvd/sendlog.c‎

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,23 +1370,28 @@ server_msg_cb(int fd, int what, void *v)
13701370
} else
13711371
#endif
13721372
{
1373+
ssize_t n;
1374+
13731375
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: reading ServerMessage", __func__);
1374-
nread = (size_t)read(fd, buf->data + buf->len, buf->size - buf->len);
1376+
n = read(fd, buf->data + buf->len, buf->size - buf->len);
1377+
if (n < 0) {
1378+
if (errno == EAGAIN || errno == EINTR)
1379+
debug_return;
1380+
sudo_warn("read");
1381+
goto bad;
1382+
}
1383+
nread = (size_t)n;
13751384
}
13761385
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: received %zd bytes from server",
13771386
__func__, nread);
1378-
switch (nread) {
1379-
case (size_t)-1:
1380-
if (errno == EAGAIN || errno == EINTR)
1381-
debug_return;
1382-
sudo_warn("read");
1383-
goto bad;
1384-
case 0:
1387+
if (nread == 0) {
13851388
if (closure->state != FINISHED)
13861389
sudo_warnx("%s", U_("premature EOF"));
13871390
goto bad;
1388-
default:
1389-
break;
1391+
}
1392+
if (nread > SIZE_MAX - buf->len) {
1393+
sudo_warnx(U_("internal error, %s overflow"), __func__);
1394+
goto bad;
13901395
}
13911396
buf->len += nread;
13921397

@@ -1496,12 +1501,17 @@ client_msg_cb(int fd, int what, void *v)
14961501
} else
14971502
#endif
14981503
{
1499-
nwritten = (size_t)write(fd, buf->data + buf->off, buf->len - buf->off);
1504+
const ssize_t n = write(fd, buf->data + buf->off, buf->len - buf->off);
1505+
if (n < 0) {
1506+
if (errno == EAGAIN || errno == EINTR)
1507+
debug_return;
1508+
sudo_warn("write");
1509+
goto bad;
1510+
}
1511+
nwritten = (size_t)n;
15001512
}
1501-
if (nwritten == (size_t)-1) {
1502-
if (errno == EAGAIN || errno == EINTR)
1503-
debug_return;
1504-
sudo_warn("write");
1513+
if (nwritten > SIZE_MAX - buf->off) {
1514+
sudo_warnx(U_("internal error, %s overflow"), __func__);
15051515
goto bad;
15061516
}
15071517
buf->off += nwritten;

0 commit comments

Comments
 (0)