On Tue, 29 Oct 2013, Sean Bruno wrote:
Log:
Queisce sign errors by using unsigned char * and casting MAP_FAILED as unsigned
char *
This increases the type errors.
Modified: head/usr.bin/xinstall/xinstall.c
==============================================================================
--- head/usr.bin/xinstall/xinstall.c Tue Oct 29 20:36:04 2013
(r257362)
+++ head/usr.bin/xinstall/xinstall.c Tue Oct 29 20:38:00 2013
(r257363)
@@ -1002,7 +1002,7 @@ compare(int from_fd, const char *from_na
int to_fd, const char *to_name __unused, size_t to_len,
char **dresp)
{
- char *p, *q;
+ unsigned char *p, *q;
int rv;
int done_compare;
DIGEST_CTX ctx;
Using plain char is fragile and often broken for the things that it
is used for in C. I happen to have been recently writing too much
about this in the comp.std.c newsgroup. Using plain char is less
fragile in POSIX than in C, since POSIX-2001 requires signed char to
be int8_t so it cannot be 1's complement or signed magnitude, or have
padding bits.
No change should be made here. Full changes to unsigned char would
require poisoning almost all char declarations by adding 'unsigned',
starting with strcpy(). Fixing strcpy()'s API was too hard for the C
standard in 1990 when C was less than half as old as now. strcpy()
is only required to use unsigned char's internally. This wasn't
explicit until C99.
@@ -1018,11 +1018,11 @@ compare(int from_fd, const char *from_na
if (trymmap(from_fd) && trymmap(to_fd)) {
p = mmap(NULL, from_len, PROT_READ, MAP_SHARED,
from_fd, (off_t)0);
- if (p == (char *)MAP_FAILED)
+ if (p == (unsigned char *)MAP_FAILED)
goto out;
q = mmap(NULL, from_len, PROT_READ, MAP_SHARED,
to_fd, (off_t)0);
- if (q == (char *)MAP_FAILED) {
+ if (q == (unsigned char *)MAP_FAILED) {
munmap(p, from_len);
goto out;
}
The casts of MAP_FAILED should be removed. They have no effect except to
break some cases. Here they mainly caused a type mismatch when the type
of the left hand operand was changed. This depends on MAP_FAILED having
the correct type (void *).
The fallback definition of MAP_FAILED in xinstall.c should also be removed.
It is compatibility cruft that was last needed in FreeBSD in 1996. FreeBSD's
MAP_FAILED had the wrong type (caddr_t) until 1997.
@@ -1036,7 +1036,7 @@ compare(int from_fd, const char *from_na
}
out:
if (!done_compare) {
- char buf1[MAXBSIZE];
+ unsigned char buf1[MAXBSIZE];
char buf2[MAXBSIZE];
int n1, n2;
It's bizarre to use unsigned chars for one buffer and still use plain
chars for the other.
I now see what this change does. digest_update() takes an unsigned
char * pointer. This makes it hard to use with normal buffers containing
char, like strcpy() would be if its pointers were unsigned char *.
The correct fix is to break digest_update()'s API like strcpy()'s, or
to cast the pointer arg to unsigned char * whenever it is called
starting with a char * arg. APIs are hard to change, so the latter
fix is normally used. Here the API is local, so it is easy to change.
digest_update() calls SHA*_Update(), and no further changes are needed
for that since it takes a void * arg and converts internally to the
correct type, which can only be unsigned char *.
Some applications are too smart and use unsigned char buffers. They
have essentially the same problem in the opposite direction with using
functions like strcpy(). I used to think that it was a bug to cast
pointer args in str*() calls, since it either has no effect or hides
type mismatch, and type mismatch may cause inconsistent comparisons.
(Note that even strcpy() needs to do comparisons for finding the null
terminator and must use unsigned char to get these right. However,
the bug is just in the API -- both the caller and the callee should
be using unsigned char for everything.
The new type mismatch between buf1 and buf2 asks for identical files
to often compare unequal, since bytes that have their "sign" bit set
become negative in buf2 but stay positive in buf1. (Files contain
bytes and bytes don't really have a sign bit, but accessing them via
buf2 corrupts them iff chars are signed.) However, there is no problem
in practice, since buf1[n] and buf2[n] are never compared directly --
all comparisons are done by calling memcmp(), and it compares bytes.
@@ -1146,7 +1146,8 @@ copy(int from_fd, const char *from_name,
{
int nr, nw;
int serrno;
- char *p, buf[MAXBSIZE];
+ unsigned char *p;
+ unsigned char buf[MAXBSIZE];
int done_copy;
DIGEST_CTX ctx;
Don't change.
@@ -1166,7 +1167,7 @@ copy(int from_fd, const char *from_name,
done_copy = 0;
if (size <= 8 * 1048576 && trymmap(from_fd) &&
(p = mmap(NULL, (size_t)size, PROT_READ, MAP_SHARED,
- from_fd, (off_t)0)) != (char *)MAP_FAILED) {
+ from_fd, (off_t)0)) != (unsigned char *)MAP_FAILED) {
nw = write(to_fd, p, size);
if (nw != size) {
serrno = errno;
Remove bogus cast, and fix indentation.
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
