diff: ftpd(8): fix for sign-compare compiler warnings

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

diff: ftpd(8): fix for sign-compare compiler warnings

Jan Klemkow
Hi,

This diff fixes some -Wsign-compare compiler warnings in ftpd(8) by
using the right types for 'i' and 'len'.  One warning is left, but I
don't see that it's fixable without suppressing the warning by a cast of
len to size_t.  And casting might be controversial in this case?!

/usr/src/libexec/ftpd/ftpd.c:2781:11: warning: comparison of integers of
different signs: 'int' and
      'unsigned long' [-Wsign-compare]
                if (len >= sizeof(buf) || len == -1) {
                    ~~~ ^  ~~~~~~~~~~~

Bye,
Jan

Index: ftpd.c
===================================================================
RCS file: /cvs/src/libexec/ftpd/ftpd.c,v
retrieving revision 1.223
diff -u -p -r1.223 ftpd.c
--- ftpd.c 3 Sep 2016 15:00:48 -0000 1.223
+++ ftpd.c 24 Nov 2018 22:26:02 -0000
@@ -390,10 +390,10 @@ main(int argc, char *argv[])
  endpwent();
 
  if (daemon_mode) {
- int *fds, i, fd;
+ int *fds, fd;
  struct pollfd *pfds;
  struct addrinfo hints, *res, *res0;
- nfds_t n;
+ nfds_t n, i;
 
  /*
  * Detach from parent.
@@ -1809,8 +1809,8 @@ statcmd(void)
  ispassive++;
  goto printaddr;
  } else if (usedefault == 0) {
- size_t alen;
- int af, i;
+ size_t alen, i;
+ int af;
 
  su = (union sockunion *)&data_dest;
 printaddr:
@@ -2545,7 +2545,8 @@ guniquefd(char *local, char **nam)
 {
  static char new[PATH_MAX];
  struct stat st;
- int count, len, fd;
+ size_t len;
+ int count, fd;
  char *cp;
 
  cp = strrchr(local, '/');

Reply | Threaded
Open this post in threaded view
|

Re: diff: ftpd(8): fix for sign-compare compiler warnings

Theo Buehler-3
On Sun, Nov 25, 2018 at 12:32:23AM +0100, Jan Klemkow wrote:
> Hi,
>
> This diff fixes some -Wsign-compare compiler warnings in ftpd(8) by
> using the right types for 'i' and 'len'.  One warning is left, but I
> don't see that it's fixable without suppressing the warning by a cast of
> len to size_t.  And casting might be controversial in this case?!

The diff looks correct to me. If anyone wants to commit

ok tb

> /usr/src/libexec/ftpd/ftpd.c:2781:11: warning: comparison of integers of
> different signs: 'int' and
>       'unsigned long' [-Wsign-compare]
>                 if (len >= sizeof(buf) || len == -1) {
>                     ~~~ ^  ~~~~~~~~~~~

This test is the wrong way around: compare CAVEATS in snprintf(3).

There's a ton of unchecked snprintfs in this code. Did you take a look
at those?

Reply | Threaded
Open this post in threaded view
|

Re: diff: ftpd(8): fix for sign-compare compiler warnings

Jan Klemkow
On Tue, Nov 27, 2018 at 09:03:15AM +0100, Theo Buehler wrote:

> On Sun, Nov 25, 2018 at 12:32:23AM +0100, Jan Klemkow wrote:
> > This diff fixes some -Wsign-compare compiler warnings in ftpd(8) by
> > using the right types for 'i' and 'len'.  One warning is left, but I
> > don't see that it's fixable without suppressing the warning by a cast of
> > len to size_t.  And casting might be controversial in this case?!
>
> The diff looks correct to me. If anyone wants to commit
>
> ok tb
>
> > /usr/src/libexec/ftpd/ftpd.c:2781:11: warning: comparison of integers of
> > different signs: 'int' and
> >       'unsigned long' [-Wsign-compare]
> >                 if (len >= sizeof(buf) || len == -1) {
> >                     ~~~ ^  ~~~~~~~~~~~
>
> This test is the wrong way around: compare CAVEATS in snprintf(3).

I missed that.  The following diff fixes that, but doesn't include
a cast.

> There's a ton of unchecked snprintfs in this code. Did you take a look
> at those?

Yes, I noticed that.  I may change that in a later diff.

Thanks,
Jan

Index: ftpd.c
===================================================================
RCS file: /cvs/src/libexec/ftpd/ftpd.c,v
retrieving revision 1.223
diff -u -p -r1.223 ftpd.c
--- ftpd.c 3 Sep 2016 15:00:48 -0000 1.223
+++ ftpd.c 6 Dec 2018 18:19:21 -0000
@@ -390,10 +390,10 @@ main(int argc, char *argv[])
  endpwent();
 
  if (daemon_mode) {
- int *fds, i, fd;
+ int *fds, fd;
  struct pollfd *pfds;
  struct addrinfo hints, *res, *res0;
- nfds_t n;
+ nfds_t n, i;
 
  /*
  * Detach from parent.
@@ -1809,8 +1809,8 @@ statcmd(void)
  ispassive++;
  goto printaddr;
  } else if (usedefault == 0) {
- size_t alen;
- int af, i;
+ size_t alen, i;
+ int af;
 
  su = (union sockunion *)&data_dest;
 printaddr:
@@ -2545,7 +2545,8 @@ guniquefd(char *local, char **nam)
 {
  static char new[PATH_MAX];
  struct stat st;
- int count, len, fd;
+ size_t len;
+ int count, fd;
  char *cp;
 
  cp = strrchr(local, '/');
@@ -2777,7 +2778,7 @@ logxfer(char *name, off_t size, time_t s
     ((guest) ? "*" : pw->pw_name), dhostname);
  free(vpw);
 
- if (len >= sizeof(buf) || len == -1) {
+ if (len == -1 || len >= sizeof(buf)) {
  if ((len = strlen(buf)) == 0)
  return; /* should not happen */
  buf[len - 1] = '\n';