Make sure compatible values have the same type. For example, when the return value of one function is used as an argument to another function, make sure they are the same type. Ensuring compatible values have the same type allows the return value to be passed as an argument to the related function without conversion, reducing the potential for conversion errors.
A source of potential errors may be traced to POSIX's tendency to overload return codes, using −1 to indicate an error condition but 0 for success and positive values as a result indicator (see ERR02-C. Avoid in-band error indicators). A good example is the read()
system call. This leads to a natural mixing of unsigned and signed quantities, potentially leading to conversion errors.
OpenSSH performs most I/O calls through a "retry on interrupt" function, atomicio()
. The following is a slightly simplified version of atomicio.c
, v 1.12 2003/07/31. The function f()
is either read()
or vwrite()
:
ssize_t atomicio(f, fd, _s, n) ssize_t (*f) (int, void *, size_t); int fd; void *_s; size_t n; { char *s = _s; ssize_t res, pos = 0; while (n > pos) { res = (f) (fd, s + pos, n - pos); switch (res) { case -1: if (errno == EINTR || errno == EAGAIN) continue; case 0: return (res); default: pos += res; } } return (pos); } |
This function has a large number of flaws. Pertinent to this recommendation, however, are the following:
atomicio()
function returns an ssize_t
(which must be a signed type). The ssize_t
type is a clear indication of poor interface design because a size should never be negative.res
and pos
are declared as ssize_t
.n - pos
results in the conversion of pos
from a signed to an unsigned type because of the usual arithmetic conversions (see INT02-C. Understand integer conversion rules).The atomicio()
function from atomicio.c
, v 1.25 2007/06/25, was modified to always return an unsigned quantity and to instead report its error via errno
:
size_t atomicio(ssize_t (*f) (int, void *, size_t), int fd, void *_s, size_t n) { char *s = _s; size_t pos = 0; ssize_t res; struct pollfd pfd; pfd.fd = fd; pfd.events = f == read ? POLLIN : POLLOUT; while (n > pos) { res = (f) (fd, s + pos, n - pos); switch (res) { case -1: if (errno == EINTR) continue; if (errno == EAGAIN) { (void)poll(&pfd, 1, -1); continue; } return 0; case 0: errno = EPIPE; return pos; default: pos += (size_t)res; } } return (pos); } |
Changes to this version of the atomicio()
function include the following:
atomicio()
function now returns a value of type size_t
.pos
is now declared as size_t
.pos += (size_t)res
now requires an explicit cast to convert from the signed return value of f()
to size_t
.n - pos
no longer requires an implicit conversion.Reducing the need to use signed types makes it easier to enable the compiler's signed/unsigned comparison warnings and fix all of the issues it reports (see MSC00-C. Compile cleanly at high warning levels).
The risk in using in-band error indicators is difficult to quantify and is consequently given as low. However, if the use of in-band error indicators results in programmers' failing to check status codes or incorrectly checking them, the consequences can be more severe.
Recommendation | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
API09-C | Low | Unlikely | High | P1 | L3 |
Tool | Version | Checker | Description |
---|---|---|---|
PC-lint Plus | 737 | Partially supported |
Search for vulnerabilities resulting from the violation of this rule on the CERT website.
Key here (explains table format and definitions)
Taxonomy | Taxonomy item | Relationship |
---|
[Miller 2007] | "Security Measures in OpenSSH" |