Date: Fri, 18 Jul 2008 14:54:56 GMT From: Christian Weisgerber <naddy@FreeBSD.org> To: freebsd-gnats-submit@FreeBSD.org Subject: ports/125750: x11/xloadimage LP64 bugs Message-ID: <200807181454.m6IEsued083502@www.freebsd.org> Resent-Message-ID: <200807181500.m6IF084t068389@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 125750
>Category: ports
>Synopsis: x11/xloadimage LP64 bugs
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-ports-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Fri Jul 18 15:00:08 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator: Christian Weisgerber
>Release: 8.0-CURRENT
>Organization:
>Environment:
FreeBSD lorvorc.mips.inka.de 8.0-CURRENT FreeBSD 8.0-CURRENT #0: Mon Jul 14 04:35:13 CEST 2008 naddy@lorvorc.mips.inka.de:/usr/obj/usr/src/sys/GENERIC amd64
>Description:
I have found a number of bugs in xloadimage 4.1-16 while examining these
warnings produced by gcc when building on 64-bit platforms:
cmuwmraster.c: In function `cmuwmIdent':
cmuwmraster.c:51: warning: comparison is always true due to limited range of data type
cmuwmraster.c: In function `cmuwmLoad':
cmuwmraster.c:94: warning: comparison is always true due to limited range of data type
cmuwmraster.c:111: warning: cast from pointer to integer of different size
Issue #1
--------
The final warning is straightfoward:
fprintf(stderr,"CMU WM raster %s is of depth %d, must be 1",
name,
(int) header.depth);
This doesn't make sense, header.depth is an array, and the third
argument should really be memToVal(header.depth, 2).
Issue #2
--------
The length parameter to the memToVal(ptr, len) macro is always given
as sizeof(type). This is pretty silly for accessing fields in
struct cmuwm_headerm, since these are defined as byte arrays. It
is also outright wrong in the case of "sizeof(long)" when a length
of 4 is intended.
Issue #3
--------
The "comparison is always true" warnings. Bear with me, this is a
tricky case of C integer type promotion. If you examine the
memToVal() macro closely for the len==4 case, the byte combining
arithmetic happens at the default type, which is int. The result
is then cast to unsigned long. The line
if (memToVal(header.magic, 4) != CMUWM_MAGIC)
really is something like
if ((unsigned long)(int expression) != 0xf10040bb)
Even if the int expression evaluates to 0xf10040bb, on a LP64
platform the cast will then cause it to be sign extended (yes!) to
0xfffffffff10040bb before the comparison happens, so the left and
right side will never compare as equal.
The simplest fix is to pull the cast into the parenthesis so that
the arithmetic expression already happens at type unsigned long.
This is in fact already the case for len==2 and len==3.
>How-To-Repeat:
>Fix:
Patch attached with submission follows:
>From naddy@openbsd.org Tue Jul 1 22:06:00 2008
Date: Tue, 1 Jul 2008 22:06:00 +0200
From: Christian Weisgerber <naddy@openbsd.org>
To: James Troup <james@nocrew.org>
Cc: jmz@FreeBSD.org
Subject: xloadimage 4.1-16: LP64 bugs
Message-ID: <20080701200600.GA39658@kemoauc.mips.inka.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.4.2.3i
Status: RO
Content-Length: 4681
Lines: 135
Dear Debian and FreeBSD port maintainers,
I have found a number of bugs in xloadimage 4.1-16 while examining these
warnings produced by gcc when building on 64-bit platforms:
cmuwmraster.c: In function `cmuwmIdent':
cmuwmraster.c:51: warning: comparison is always true due to limited range of data type
cmuwmraster.c: In function `cmuwmLoad':
cmuwmraster.c:94: warning: comparison is always true due to limited range of data type
cmuwmraster.c:111: warning: cast from pointer to integer of different size
I have appended a patch below. Please review, and if you find the
fixes correct, apply them to xloadimage in your projects.
Here is a discussion of the problems:
Issue #1
--------
The final warning is straightfoward:
fprintf(stderr,"CMU WM raster %s is of depth %d, must be 1",
name,
(int) header.depth);
--- cmuwmraster.c.orig Tue Jul 1 19:08:24 2008
+++ cmuwmraster.c Tue Jul 1 19:08:57 2008
@@ -22,9 +22,9 @@ struct cmuwm_header *headerp;
{
printf("%s is a %ldx%ld %ld plane CMU WM raster\n",
name,
- memToVal(headerp->width, sizeof(long)),
- memToVal(headerp->height, sizeof(long)),
- memToVal(headerp->depth, sizeof(short)));
+ memToVal(headerp->width, 4),
+ memToVal(headerp->height, 4),
+ memToVal(headerp->depth, 2));
}
int cmuwmIdent(fullname, name)
@@ -48,7 +48,7 @@ char *fullname, *name;
break;
case sizeof(struct cmuwm_header):
- if (memToVal(header.magic, sizeof(long)) != CMUWM_MAGIC)
+ if (memToVal(header.magic, 4) != CMUWM_MAGIC)
{
r = 0;
break;
@@ -91,7 +91,7 @@ unsigned int verbose;
exit(1);
case sizeof(struct cmuwm_header):
- if (memToVal(header.magic, sizeof(long)) != CMUWM_MAGIC)
+ if (memToVal(header.magic, 4) != CMUWM_MAGIC)
{
zclose(zf);
return(NULL);
@@ -104,16 +104,16 @@ unsigned int verbose;
return(NULL);
}
- if (memToVal(header.depth, sizeof(short)) != 1)
+ if (memToVal(header.depth, 2) != 1)
{
fprintf(stderr,"CMU WM raster %s is of depth %d, must be 1",
name,
- (int) header.depth);
+ memToVal(header.depth, 2));
return(NULL);
}
- image = newBitImage(width = memToVal(header.width, sizeof(long)),
- height = memToVal(header.height, sizeof(long)));
+ image = newBitImage(width = memToVal(header.width, 4),
+ height = memToVal(header.height, 4));
linelen = (width / 8) + (width % 8 ? 1 : 0);
lineptr = image->data;
--- image.h.orig Tue Jul 1 21:18:52 2008
+++ image.h Tue Jul 1 21:21:24 2008
@@ -163,7 +163,7 @@ typedef struct {
((LEN) == 2 ? ((unsigned long) \
(*(byte *)(PTR) << 8) | \
(*((byte *)(PTR) + 1))) : \
- ((unsigned long)((*(byte *)(PTR) << 24) | \
+ (((unsigned long)(*(byte *)(PTR) << 24) | \
(*((byte *)(PTR) + 1) << 16) | \
(*((byte *)(PTR) + 2) << 8) | \
(*((byte *)(PTR) + 3)))))))
@@ -176,7 +176,7 @@ typedef struct {
(*((byte *)(PTR) + 2) << 16)) : \
((LEN) == 2 ? ((unsigned long) \
(*(byte *)(PTR)) | (*((byte *)(PTR) + 1) << 8)) : \
- ((unsigned long)((*(byte *)(PTR)) | \
+ (((unsigned long)(*(byte *)(PTR)) | \
(*((byte *)(PTR) + 1) << 8) | \
(*((byte *)(PTR) + 2) << 16) | \
(*((byte *)(PTR) + 3) << 24))))))
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200807181454.m6IEsued083502>