Build failure with gcc 7.0.1

[scop]

Build fails with current Fedora rawhide, gcc 7.0.1:

attach.c: In function ‘setup_steal_socket’:
attach.c:393:15: error: ‘%s’ directive output may be truncated writing up to 4095 bytes into a region of size 108 [-Werror=format-truncation=]
              "%s/reptyr.sock", steal->tmpdir);
               ^~
attach.c:392:5: note: ‘snprintf’ output between 13 and 4108 bytes into a destination of size 108
     snprintf(steal->addr_un.sun_path, sizeof(steal->addr_un.sun_path),
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "%s/reptyr.sock", steal->tmpdir);
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

[scop]

...and there are a few more:

platform/linux/linux.c: In function ‘get_terminal_state’:
platform/linux/linux.c:232:13: error: In the GNU C Library, "major" is defined
 by <sys/sysmacros.h>. For historical compatibility, it is
 currently defined by <sys/types.h> as well, but we plan to
 remove this soon. To use "major", include <sys/sysmacros.h>
 directly. If you did not intend to use a system-defined macro
 "major", you should undefine it after including <sys/types.h>. [-Werror]
     if (major(steal->target_stat.ctty) != UNIX98_PTY_SLAVE_MAJOR) {
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                                                                                                                                                                                                                 
platform/linux/linux.c: In function ‘find_master_fd’:
platform/linux/linux.c:270:13: error: In the GNU C Library, "makedev" is defined
 by <sys/sysmacros.h>. For historical compatibility, it is
 currently defined by <sys/types.h> as well, but we plan to
 remove this soon. To use "makedev", include <sys/sysmacros.h>
 directly. If you did not intend to use a system-defined macro
 "makedev", you should undefine it after including <sys/types.h>. [-Werror]
         if (st.st_rdev != PTMX_DEVICE)
             ^~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                                                                                                                                                                                                                                                    
platform/linux/linux.c:290:13: error: In the GNU C Library, "minor" is defined
 by <sys/sysmacros.h>. For historical compatibility, it is
 currently defined by <sys/types.h> as well, but we plan to
 remove this soon. To use "minor", include <sys/sysmacros.h>
 directly. If you did not intend to use a system-defined macro
 "minor", you should undefine it after including <sys/types.h>. [-Werror]
         if (ptn == (int)minor(steal->target_stat.ctty)) {
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                            

[nelhage]

Hi,

Can you confirm if the gcc7 branch builds for you? It builds using my gcc-7 from ubuntu-toolchain-r/test, but it'd be good to get confirmation.

Thanks for the report!

[scop]

Yep, the gcc7 branch builds for me.

[nelhage]

Thanks for the report! I'd love to get this in CI to make sure we don't regress, but I don't see an easy way to get gcc-7 packages in travis right now.

[scop]

One way to get gcc7 and pretty much anything would be to use docker in Travis, for inspiration see e.g. what I've done in https://github.com/rpm-software-management/rpmlint (not necessarily the prettiest, but has worked for me).

[rekm]

Hi, i just tried packaging tag 0.6.2 into a conda package and ran into the same issue, referenced in OPs first post.
Isn't this line always false, because of sprints innate truncation.

if (snprintf(steal->addr_un.sun_path, sizeof(steal->addr_un.sun_path),
                "%s/reptyr.sock", steal->tmpdir) >= sizeof(steal->addr_un.sun_path)) 
{
    error("tmpdir path too long!");
    return ENAMETOOLONG;
}

I hard coded a patch for ubuntu/debian systems by truncating explicitly,
where addr_un.sun_path = 108.

snprintf(steal->addr_un.sun_path, sizeof(steal->addr_un.sun_path),
             "%.%s/reptyr.sock", steal->tmpdir, sizeof(steal->addr_un.sun_path)-(12+1)) 
              && strlen(steal->addr_un.sun_path)+1 >= sizeof(steal->addr_un.sun_path)

This version of snprintf obviously lacking type safety because sizeof() returns size_t.
My two cents, that might be based on my erroneous understanding of the snprintf function.

Edit: (couldn't stop myself)

... line 393 
%.*s/reptyr.sock",(int)sizeof(steal->addr_un.sun_path)-13, steal->tmpdir) 
...

[nelhage]

snprintf is documented in my man pages as

       The functions snprintf() and vsnprintf() do not write  more  than  size
       bytes  (including the terminating null byte ('\0')).  If the output was
       truncated due to this limit, then the return value  is  the  number  of
       characters  (excluding the terminating null byte) which would have been
       written to the final string if enough space had been available.   Thus,
       a  return  value  of  size or more means that the output was truncated.
       (See also below under NOTES.)

Which suggests to me that this comparison is a valid way to check for truncation.

I'm not really sure what the right fix is here. sun_path is fixed size and I can't change that, and as far as I can tell I'm correctly using snprintf to avoid overflow, and detect the truncation and error out, which is pretty much the only correct thing I can do if the path is too long.

[rekm]

Yeah you are absolutely right.
Surprisingly mine says exactly the same. The compiler warning seems unfitting for the snprint family of functions. It actually was, me not understanding the snprintf() function.

Also my truncating before using snprintf() and checking the size afterwards is really redundant.
This redundancy was forced onto me by conda and should not concern the main project.

This tool works like a charm by the way.
Thanks for indulging me.