[PATCH 0/1] Fix -Wformat-overflow= warnings in zftp.c

[PATCH 0/1] Fix -Wformat-overflow= warnings in zftp.c

[Wesley Schwengle]

Hello all,

I was compiling zsh from master on my Debian testing box with cc 8.3.0
and I got the following warning:

zftp.c: In function ‘zfstats’:
zftp.c:1265:26: warning: ‘%02d’ directive writing between 2 and 11 bytes
into a region of size between 9 and 16 [-Wformat-overflow=]
sprintf(tmbuf, "%04d%02d%02d%02d%02d%02d",
^~~~
zftp.c:1265:21: note: directive argument in the range [-2147483647, 2147483647]
sprintf(tmbuf, "%04d%02d%02d%02d%02d%02d",
^~~~~~~~~~~~~~~~~~~~~~~~~~
zftp.c:1265:6: note: ‘sprintf’ output between 15 and 67 bytes into a
destination of size 20
sprintf(tmbuf, "%04d%02d%02d%02d%02d%02d",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tm->tm_year + 1900, tm->tm_mon+1, tm->tm_mday,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tm->tm_hour, tm->tm_min, tm->tm_sec);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I made a small patch that resolves the warning.

Cheers,
Wesley

Wesley Schwengle (1):
  Change handrolled strftime to strftime in zftp.c

 Src/Modules/zftp.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

-- 
2.21.0.196.g041f5ea1cf

[PATCH] Change handrolled strftime to strftime in zftp.c

[Wesley Schwengle]

From: Wesley Schwengle <wesley@schwengle.net>

Compiling zsh on Debian testing/unstable with cc (Debian 8.3.0-2) 8.3.0
emits a -Wformat-overflow= warning on line 1265 in zftp.c. Replace the
code with strftime() resolves it.

Signed-off-by: Wesley Schwengle <wesley@schwengle.net>
---
 Src/Modules/zftp.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index 4aaa1f072..e0789216a 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -1257,14 +1257,7 @@ zfstats(char *fnam, int remote, off_t *retsize, char **retmdtm, int fd)
 	if (retmdtm) {
 	    /* use gmtime() rather than localtime() for consistency */
 	    tm = gmtime(&statbuf.st_mtime);
-	    /*
-	     * FTP format for data is YYYYMMDDHHMMSS
-	     * Using tm directly is easier than worrying about
-	     * incompatible strftime()'s.
-	     */
-	    sprintf(tmbuf, "%04d%02d%02d%02d%02d%02d",
-		    tm->tm_year + 1900, tm->tm_mon+1, tm->tm_mday,
-		    tm->tm_hour, tm->tm_min, tm->tm_sec);
+	    strftime(tmbuf, 15, "%Y%m%d%H%M%S", tm);
 	    mt = ztrdup(tmbuf);
 	}
     }
-- 
2.21.0.196.g041f5ea1cf

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Jun T]

> 2019/03/23 19:11, Wesley Schwengle <wesleyschwengle@gmail.com> wrote:
> 
> +	    strftime(tmbuf, 15, "%Y%m%d%H%M%S", tm);

zsh has its own ztrftime() (which works on systems without strftime())
and we can use it here.

I tried it and of course it worked, but I noticed another (unrelated)
problem. If I build zsh with --enable-zsh-debug, and test on my Mac,

% zmodload zsh/zftp
% zftp local flame
 zftp.c:2519: Shell compiled with wrong off_t size

On Darwin (and 64 bit Linux) both long and off_t are 64 bit. In
configure.ac, line 975 and below, it says OFF_T_IS_64_BIT is
defined only if LONG_IS_64_BIT is NOT defined (i.e., long is 32 bit)
and off_t is 64 bit. But in zftp.c, line 2518 (before patch),


#ifdef OFF_T_IS_64_BIT
        printf("%s %s\n", output64(sz), mt);
#else
        DPUTS(sizeof(sz) > 4, "Shell compiled with wrong off_t size");
        printf("%ld %s\n", (long)sz, mt);
#endif


Should we replace "#ifdef OFF_T_IS_64_BIT" by "#ifndef OFF_T_IS_64_BIT"?

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Peter Stephenson]

On Mon, 2019-03-25 at 21:13 +0900, Jun T wrote:
> #ifdef OFF_T_IS_64_BIT
>         printf("%s %s\n", output64(sz), mt);
> #else
>         DPUTS(sizeof(sz) > 4, "Shell compiled with wrong off_t size");
>         printf("%ld %s\n", (long)sz, mt);
> #endif
> 
> 
> Should we replace "#ifdef OFF_T_IS_64_BIT" by "#ifndef OFF_T_IS_64_BIT"?

The obvious test would be

#ifdef ZSH_64_BIT_TYPE

which is how we know output64 is defined.  That's used in some other case.

Cheers
pws

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Jun T.]

> 2019/03/25 21:42, Peter Stephenson <p.stephenson@samsung.com> wrote:
>>  
>> Should we replace "#ifdef OFF_T_IS_64_BIT" by "#ifndef OFF_T_IS_64_BIT"?
> 
> The obvious test would be
> 
> #ifdef ZSH_64_BIT_TYPE

But ZSH_64_BIT_TYPE is defined only if long is 32 bit
so we still get 'Shell compiled with wrong off_t size'.
Do you mean "#ifndef ZSH_64_BIT_TYPE"?

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Jun T.]

Sorry, the problem is not the OFF_T_IS_64_BIT but the comparison
in DPUTS().

I will push Wesley Schwengle's patch (with strftime --> ztrftime)
without the following because the two are no related.


diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index 4aaa1f072..b3fbeae17 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -2518,7 +2518,8 @@ zftp_local(UNUSED(char *name), char **args, int flags)
 #ifdef OFF_T_IS_64_BIT
 	printf("%s %s\n", output64(sz), mt);
 #else
-	DPUTS(sizeof(sz) > 4, "Shell compiled with wrong off_t size");
+	DPUTS(sizeof(sz) > sizeof(long), 
+	      "Shell compiled with wrong off_t size");
 	printf("%ld %s\n", (long)sz, mt);
 #endif
 	zsfree(mt);

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Peter Stephenson]

> On 25 March 2019 at 13:07 "Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> > 2019/03/25 21:42, Peter Stephenson <p.stephenson@samsung.com> wrote:
> >>  
> >> Should we replace "#ifdef OFF_T_IS_64_BIT" by "#ifndef OFF_T_IS_64_BIT"?
> > 
> > The obvious test would be
> > 
> > #ifdef ZSH_64_BIT_TYPE
> 
> But ZSH_64_BIT_TYPE is defined only if long is 32 bit
> so we still get 'Shell compiled with wrong off_t size'.
> Do you mean "#ifndef ZSH_64_BIT_TYPE"?

I don't see how that could work since output64() wouldn't exist.

I think this means the error message in the other branch is now redundant --- if
long is a 64-bit value it's perfectly OK to use that code.  Could change the DPUTS
check to check sizeof(sz) for that purpose instead?

pws

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Jun T.]

> 2019/03/25 23:36, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:

> Could change the DPUTS
> check to check sizeof(sz) for that purpose instead?

Yes, as in my previous post (mailing list has some delay).

A similar fix for stat.c is
worker/20823
commit d7c13fb2c3b1b014acde9c1cb17a1e34239b9751

Re: [PATCH] Change handrolled strftime to strftime in zftp.c

[Peter Stephenson]

On Mon, 2019-03-25 at 14:36 +0000, Peter Stephenson wrote:
> > 
> > On 25 March 2019 at 13:07 "Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> > > 
> > > 2019/03/25 21:42, Peter Stephenson <p.stephenson@samsung.com> wrote:
> > > > 
> > > >  
> > > > Should we replace "#ifdef OFF_T_IS_64_BIT" by "#ifndef OFF_T_IS_64_BIT"?
> > > The obvious test would be
> > > 
> > > #ifdef ZSH_64_BIT_TYPE
> > But ZSH_64_BIT_TYPE is defined only if long is 32 bit
> > so we still get 'Shell compiled with wrong off_t size'.
> > Do you mean "#ifndef ZSH_64_BIT_TYPE"?
> I don't see how that could work since output64() wouldn't exist.
> 
> I think this means the error message in the other branch is now
> redundant --- if long is a 64-bit value it's perfectly OK to use that
> code.  Could change the DPUTS check to check sizeof(sz) for that
> purpose instead?

...which you've now done, so this should be fine.

pws