Bug 1192 - warning: comparison between signed and unsigned
Summary: warning: comparison between signed and unsigned
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: scp (show other bugs)
Version: 4.3p2
Hardware: amd64 All
: P2 normal
Assignee: Assigned to nobody
URL: http://www.openbsd.org/cgi-bin/cvsweb...
Keywords:
Depends on:
Blocks: V_5_1
  Show dependency treegraph
 
Reported: 2006-05-22 13:31 AEST by Carlo Marcelo Arenas Belon
Modified: 2008-07-22 12:08 AEST (History)
1 user (show)

See Also:


Attachments
reverting first snippet for scp.c version 1.122 (503 bytes, patch)
2006-05-22 13:36 AEST, Carlo Marcelo Arenas Belon
no flags Details | Diff
make amt size_t and add cast to prevent warnings. (1.44 KB, patch)
2008-06-14 03:29 AEST, Darren Tucker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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