Bug 1192

Summary:

warning: comparison between signed and unsigned

Product:

Portable OpenSSH

Reporter:

Carlo Marcelo Arenas Belon <carenas>

Component:

scp

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

normal

CC:

dtucker

Priority:

Version:

4.3p2

Hardware:

amd64

OS:

All

URL:

http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/scp.c.diff?r1=1.121&r2=1.122&f=h

Bug Depends on:

Bug Blocks:

1452

Attachments:

Description	Flags
reverting first snippet for scp.c version 1.122	none
make amt size_t and add cast to prevent warnings.	none

Description Carlo Marcelo Arenas Belon 2006-05-22 13:31:49 AEST

introduced as part of scp version 1.122 (OpenBSD 3.8/OpenSSH 4.2), and which results in the following warning when compiled with -Wsign-compare :

scp.c: In function `source':
scp.c:632: warning: comparison between signed and unsigned
scp.c:639: warning: comparison between signed and unsigned

the problem is that off_t is defined signed (long long for OpenBSD and long int for linux/amd64), while size_t is unsigned (unsigned long for OpenBSD and long unsigned int for Linux).

for LPI32 architectures (like i386), the expansion from unsigned long (4 bytes) to long long (8 bytes) is safe, but no so for LP64 architectures (like amd64) where "long long" and "unsigned long" are 8 bytes wide.

Comment 1 Carlo Marcelo Arenas Belon 2006-05-22 13:36:47 AEST

Created attachment 1142 [details]
reverting first snippet for scp.c version 1.122

compiling without warnings and validated for OpenBSD/amd64, OpenBSD/i386 and Gentoo Linux 2006.0/x86_64

Comment 2 Darren Tucker 2006-05-22 13:55:16 AEST

Comment on attachment 1142 [details]
reverting first snippet for scp.c version 1.122

>-	size_t result;
>+	off_t i, amt, result, statbytes;

I don't think that's the right way forward since later there is:

        result = atomicio(read, fd, bp->buf, amt)

and atomicio now returns a size_t.

Instead, I think we should be changing the progressmeter interface to u_int64_t from off_t (since ultimately, the size it operates on is fixed at 64 bits by the packet format of the sftp spec).  This would change "amt" and also fix the warning.

There's an old bug for this (#842) but I suspect the diff will need some tweaking.

Comment 3 Darren Tucker 2006-05-22 14:01:00 AEST

Err, ignore comment #2.  It's a similar but unrelated issue (but in the same chunk of code).

Comment 4 Darren Tucker 2008-01-01 03:11:02 AEDT

The code in question has been removed from the current development version so these warnings should now be gone.

Could you please confirm by testing a development snapshot from http://www.mindrot.org/openssh_snap/ ?  Thanks.

Comment 5 Darren Tucker 2008-06-14 03:29:06 AEST

Created attachment 1519 [details]
make amt size_t and add cast to prevent warnings.

I've now got access to an OpenBSD/amd64 system and I see that this is still present.  This patch should fix it.

Comment 6 Darren Tucker 2008-06-14 04:59:31 AEST

This has been applied and will be in the next release.

Thanks.

Comment 7 Damien Miller 2008-07-22 12:08:21 AEST

Mass update RESOLVED->CLOSED after release of openssh-5.1