Fixed compiler warnings/errors about snprintf truncations in log.c

[matt335672]

Compiling the current development wavefront on Ubuntu 20.04 (gcc 9.3.0) results in the following errors:-

log.c: In function ‘log_message’:
log.c:558:30: error: ‘%.2d’ directive output may be truncated writing between 2 and 11 bytes into a region of size between 9 and 16 [-Werror=format-truncation=]
  558 |     snprintf(buff, 21, "[%.4d%.2d%.2d-%.2d:%.2d:%.2d] ", now->tm_year + 1900,
      |                              ^~~~
log.c:558:24: note: directive argument in the range [-2147483647, 2147483647]
  558 |     snprintf(buff, 21, "[%.4d%.2d%.2d-%.2d:%.2d:%.2d] ", now->tm_year + 1900,
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/stdio.h:867,
                 from log.c:28:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:67:10: note: ‘__builtin___snprintf_chk’ output between 21 and 73 bytes into a destination of size 21
   67 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |        __bos (__s), __fmt, __va_arg_pack ());
      |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

These errors used to be warnings. However commit 7e58209 (not in the last release) applies -Werror to the compilation.

This fix simply restricts the values passed to %.2d by using the modulus operator '%'.

We possibly need this for v0.9.14 to prevent the Ubuntu 20.04 builds from breaking.

Thanks to @endofreal for raising this as a side issue in #1658

[aquesnel]

Hi @matt335672 ,

I reproduce the compiler error in this pull request in a Travis CI build using the same version of Ubuntu as was used in #1658 :
aquesnel@4bf3161
https://travis-ci.org/github/aquesnel/xrdp/builds/720506442

I also confirmed that your commit in this pull request fixes the build:
https://travis-ci.org/github/aquesnel/xrdp/builds/720508560

Do you think I should make a pull request for my change to include Ubuntu 20.04 in the Travis CI builds to get the newer gcc checks run on the code?

[matt335672]

Hi @aquesnel

If you do that, you'll be dependent on this fix being pulled into devel.

Either of the two could be preferable I think:-

• You could raise a separate PR to bump the distro version to focal.
• I could add the version bump into this PR to demonstrate the problem has been fixed.

I've had a look at your build_ubuntu_focal branch change. The comment I would make is that I think it's worth bumping the distro version for all the builds and not just the gcc one. That way you'll get later versions of clang and g++ running too.

Thoughts?

[aquesnel]

Hi @matt335672 ,

I don't want to block this pull request unnecessarily, so I'm happy to submit a follow up pull request with the distro version bump, or have you merge/squash the change into this pull request.

I think it's a good idea to bump the distro version for all the compilers in the build.

My only hesitation at bumping the distro version is that I don't know if there is a minimum distro or toolchain version that is being targeted that would lose coverage if we bumped the distro version for all builds. I didn't find anything about minium supported versions, but I did find wiki page for compiling on Debian 6 and RedHat 6. https://github.com/neutrinolabs/xrdp/wiki/Building-on-Debian-6

Do you know if there is a minimum toolchain or distro version that is targeted by this project?

[matt335672]

I don't actually know what the policy is supporting older distros or toolchains. Off the top-of-my-head RHEL 6 won't actually build the latest v0.9.x version of XRDP as autoconf is too old. My suspicion is that a major feature update which would prevent building on a current platform would result in a major version bump. Is that right @metalefty?

My own thinking is that using the latest compilers possible to do the regression error checking is the best approach. It's probably unlikely that an older compiler would find a problem that a newer compiler woudn't.

Currently v0.9.14 is imminent and so at the moment I'd vote for a follow-up PR after than happens.

[metalefty]

Sorry for the delay. We don't have explicit rules for minimum OS & toolchain. However, we would not support EoL'ed OS. Regarding RHEL6 (or CentOS 6), it will reach EoL soon so I no longer spend time on it. People who want to maintain xrdp on such OS should contribute. We accept such contributions.

My suspicion is that a major feature update which would prevent building on a current platform would result in a major version bump. Is that right @metalefty

There are no explicit rules on this too but I think it should be done like that.

The existence of building documents for older OSs not necessarily implies such OSs are supported. Wiki is community-driven knowledge base. If it is helpful for someone, I don't want to delete old pages. It old pages are noise, we should do garbage collection.

[metalefty]

BTW, based on the knowledge developing X11RDP-RH-Matic (no longer mainteined), I know autorecon268 is needed on EL6.

[matt335672]

I must admit I was in a bit of a rush when I put this one together. Although it's a minimum change from where we are, I think using strftime() would be a much cleaner way to do this. I'm out of time today, but I'll look at it tomorrow.

[pdynarowski]

same problem on archlinux (gcc 10)

[matt335672]

I've used strftime() instead, and the code's a lot cleaner. I've used ANSI C features only for maximum compatibility. Code builds OK on gcc 9.3.0 (Xubuntu 20) as well as the standard platforms.

@metalefty - can you give this a quick sanity check? If you're happy I'll squash and merge it.

Thanks.

[metalefty]

LGTM. I think strftime is exactly for such purpose. Squash it.