nosh: G++ warnings

From: Guillermo <gdiazhartusch_at_gmail.com>
Date: Sun, 14 Aug 2016 14:52:54 -0300

Hello,

Since the source package usually builds fine regardless, I have
previously ignored the warnings. But while doing the upgrade to
version 1.28 I decided to have a closer look. A majority of them are
about ignoring the result of some POSIX library function calls, as a
side effect of GNU libc marking them with __attribute__
((warn_unused_result)). But others I thought should be reported in
case something needs to get fixed.

(BTW, this is Gentoo-packaged G++ 4.9.3 on x86 architecture, i.e.
32-bit GNU/Linux)

1) Writing to unallocated memory

warning: array subscript is above array bounds [-Warray-bounds]

kqueue_linux.cpp: In function 'int kevent_linux(int, const kevent*,
int, kevent*, int, const timespec*)':
   p[1].events = POLLIN;

Object p is declared with type struct pollfd[1], so likely a bug.

warning: (this didn't actually trigger one, it was just code analysis)

builtins.cpp: In function 'void safe_execvp(const char*, const char**)':
  const size_t plen = std::strlen(prog);
  std::auto_ptr<char> buf(new(std::nothrow) char [std::strlen(path) +
1 + plen + 1]);
  //...
  for (;;) {
    const char * colon = std::strchr(path, ':');
    if (!colon) colon = std::strchr(path, '\0');
      size_t l(colon - path);
    if (!l)
      buf.get()[l++] = '.';
    else
      memcpy(buf.get(), path, l);
    buf.get()[l++] = '/';
    memcpy(buf.get() + l, prog, plen + 1);
    //...
    path = colon + 1
  }

If I am not mistaken, if colon - path == 0, then path points to "",
std::strlen(path) returns 0, and buf.get() points to an array of plen
+ 2 char objects. But then, doesn't prepending "./" to prog result in
a string of length plen + 3? So it is a bug?

2) std::fprintf() conversion specifications not matching argument type
on 32-bit machines

warning: format '%lu' expects argument of type 'long unsigned int',
but argument N has type 'uintptr_t {aka unsigned int}' [-Wformat=]

export-to-rsyslog.cpp: In function 'void export_to_rsyslog(const
char*&, std::vector<const char*>&)':
  std::fprintf(stderr, "DEBUG: vnode event ident %lu fflags %x\n",
e.ident, e.fflags);
  std::fprintf(stderr, "DEBUG: event filter %hd ident %lu fflags
%x\n", e.filter, e.ident, e.fflags);

initctl-read.cpp: In function 'void initctl_read(const char*&,
std::vector<const char*>&)':
  std::fprintf(stderr, "%s: DEBUG: read event ident %lu\n", prog, e.ident);
  std::fprintf(stderr, "%s: DEBUG: signal event ident %lu fflags
%x\n", prog, e.ident, e.fflags);
  std::fprintf(stderr, "%s: DEBUG: event filter %hd ident %lu fflags
%x\n", prog, e.filter, e.ident, e.fflags);

klog-read.cpp: In function 'void klog_read(const char*&,
std::vector<const char*>&)':
  std::fprintf(stderr, "%s: DEBUG: read event ident %lu\n", prog, e.ident);
  std::fprintf(stderr, "%s: DEBUG: signal event ident %lu fflags
%x\n", prog, e.ident, e.fflags);
  std::fprintf(stderr, "%s: DEBUG: event filter %hd ident %lu fflags
%x\n", prog, e.filter, e.ident, e.fflags);

service-dt-scanner.cpp: In function 'void service_dt_scanner(const
char*&, std::vector<const char*>&)':
  std::fprintf(stderr, "%s: DEBUG: vnode event ident %lu fflags %x\n",
prog, e.ident, e.fflags);
  std::fprintf(stderr, "%s: DEBUG: event filter %hd ident %lu fflags
%x\n", prog, e.filter, e.ident, e.fflags);

syslog-read.cpp: In function 'void syslog_read(const char*&,
std::vector<const char*>&)':
  std::fprintf(stderr, "%s: DEBUG: read event ident %lu\n", prog, e.ident);
  std::fprintf(stderr, "%s: DEBUG: signal event ident %lu fflags
%x\n", prog, e.ident, e.fflags);
  std::fprintf(stderr, "%s: DEBUG: event filter %hd ident %lu fflags
%x\n", prog, e.filter, e.ident, e.fflags);

warning: format '%lu' expects argument of type 'long unsigned int',
but argument 5 has type 'size_t {aka unsigned int}' [-Wformat=]

service-manager.cpp: In function 'void control_message(int)':
  std::fprintf(stderr, "%s: WARNING: unknown control message command
%u with %lu file descriptors\n", prog, m.command, count_fds);

warning: format '%lu' expects argument of type 'long unsigned int',
but argument 5 has type 'std::map<index, service>::size_type {aka
unsigned int}' [-Wformat=]

service-manager.cpp: In function 'void service_manager(const char*&,
std::vector<const char*>&)':
  std::fprintf(stderr, "%s: INFO: %s %lu %s\n", prog, "Shutdown
requested but there are", services.size(), "services still active.");

Bugs I think. As far as I know, this results in undefined behaviour, and:

* there are standard conversion specifications for std::size_t values:
%zu, %zo, %zx, etc.
* macros PRIuPTR, PRIxPTR, etc. from <cinttypes> can be used to
construct portable conversion specifications for std::uintptr_t
values.
* std::map<>::size() returns std::map<>::size_type, which is either an
unspecified or implementation-defined type, I don't know which.

But I have to ask, why not go full C++, do 'std::cerr << whatever'
(everywhere, not just here) instead of 'std::fprintf(stderr, format,
whatever)', and let the compiler figure out the correct 'operator
<<()' function to print values of the involved types? After all, there
are other places in the code where << is used on std::cout, and on
std::ofstream and std::ostringstream objects, so there doesn't seem to
be an objection to it :)

3) Type-punned pointers and strict-aliasing

warning: dereferencing type-punned pointer will break strict-aliasing
rules [-Wstrict-aliasing]

initctl-read.cpp: In function 'void initctl_read(const char*&,
std::vector<const char*>&)':
      const Client::initreq & r(*reinterpret_cast<const
Client::initreq *>(c.buffer));

kqueue_linux.cpp: In member function 'int
{anonymous}::Queue::wait(kevent*, int, const timespec*)':
     const struct signalfd_siginfo & s(*reinterpret_cast<const struct
signalfd_siginfo *>(signal_buf));
     const struct inotify_event & ie(*reinterpret_cast<const struct
inotify_event *>(notify_buf));

Other than figuring out that it is somehow related to a an
optimization enabled by -fstrict-aliasing (in turn autoenabled by
certain -O settings), I don't know what to make of this. I still don't
completely understand what 'breaking strict-aliasing' means :P

4) Null pointer constant of type int as sentinel

warning: missing sentinel in function call [-Wformat=]

emergency-login.cpp: In function 'void emergency_login(const char*&,
std::vector<const char*>&)':
  execl(shell, shell, 0);

I suppose that G++ does the right thing here anyway, but isn't passing
an int argument for which there is no parameter (i.e. matching the
ellipsis in the prototype) to a function that expects a pointer
undefined behaviour? Even if the argument is a null pointer constant?
I believe the pretty way to write the sentinel is (const char *)0 in
C, and nullptr in C++ (>=2011).

5) Using std::auto_ptr<>

warning: 'auto_ptr' is deprecated (declared at
/usr/lib/gcc/i686-pc-linux-gnu/4.9.3/include/g++-v4/backward/auto_ptr.h:87)
[-Wdeprecated-declarations]

builtins.cpp: In function 'void safe_execvp(const char*, const char**)':
  std::auto_ptr<char> buf(new(std::nothrow) char [std::strlen(path) +
1 + plen + 1]);

service-show.cpp: In function 'std::string get_log(int)':
  std::auto_ptr<char> buf(new(std::nothrow) char [s.st_size]);

Still supported, but perhaps it is time to switch to
std::unique_ptr<>? :) It looks like auto_ptr is going to be completely
removed in the 2017 standard.

Thanks,
G.
Received on Sun Aug 14 2016 - 17:52:54 UTC

This archive was generated by hypermail 2.3.0 : Sun May 09 2021 - 19:44:19 UTC