ClusterLabs / cluster-glue

Reusable Cluster Components ("glue")
http://clusterlabs.org/
GNU General Public License v2.0
18 stars 28 forks source link

New error with GCC7 #10

Closed marxin closed 7 years ago

marxin commented 7 years ago

Building the project with GCC7, I see:

[ 59s] cl_pidfile.c:172:29: error: 'builtin_snprintf_chk' output may be truncated before the last format character [-Werror=format-truncation=] [ 59s] snprintf(buf, sizeof(buf), "%*ld\n", LOCKSTRLEN-1, mypid); [ 59s] ^~~~ [ 59s] In file included from /usr/include/stdio.h:939:0, [ 59s] from cl_pidfile.c:18: [ 59s] /usr/include/bits/stdio2.h:64:10: note: 'builtin_snprintf_chk' output between 12 and 13 bytes into a destination of size 12 [ 59s] return builtin___snprintf_chk (s, n, USE_FORTIFY_LEVEL - 1, [ 59s] ^~~~~~~~~~~~~~~~ [ 59s] bos (s), fmt, va_arg_pack ()); [ 59s] ~~~~~~~~~

Which can be easily fixed by increasing LOCKSTRLEN from 11 to 12, which is enough even for 64-bit long numbers. I can prepare pull request whether it's preferred way how to fix the issue?

dmuhamedagic commented 7 years ago

On Mon, Mar 20, 2017 at 06:17:59AM -0700, marxin wrote:

Building the project with GCC7, I see:

[ 59s] cl_pidfile.c:172:29: error: 'builtin_snprintf_chk' output may be truncated before the last format character [-Werror=format-truncation=] [ 59s] snprintf(buf, sizeof(buf), "%*ld\n", LOCKSTRLEN-1, mypid); [ 59s] ^~~~ [ 59s] In file included from /usr/include/stdio.h:939:0, [ 59s] from cl_pidfile.c:18: [ 59s] /usr/include/bits/stdio2.h:64:10: note: 'builtin_snprintf_chk' output between 12 and 13 bytes into a destination of size 12 [ 59s] return builtin___snprintf_chk (s, n, USE_FORTIFY_LEVEL - 1, [ 59s] ^~~~~~~~~~~~~~~~ [ 59s] bos (s), fmt, va_arg_pack ()); [ 59s] ~~~~~~~~~

Which can be easily fixed by increasing LOCKSTRLEN from 11 to 12, which is enough even for 64-bit long numbers. I can prepare pull request whether it's preferred way how to fix the issue?

Yes, please. LOCKSTRLEN is used only for automatic variables, hence we don't need to think about the library compatibility issues.

There's just one awkward test in ./lib/clplumbing/cl_pidfile.c:

137         if (fstat(fd, &sbuf) >= 0 && sbuf.st_size < LOCKSTRLEN) {
138             sleep(1); /* if someone was about to create one,
139                    * give'm a sec to do so

If someone's unlucky there should only be an extra 1 second sleep.

lge commented 7 years ago

Seeing this only now. At least the comment in the reasoning above is wrong: 64 bit integers may well need 20 bytes (signed long int max, 0x7fffffffffffffff, 9223372036854775807) or 21 byte (signed long int min, -9223372036854775808, or unsigned 18446744073709551615). Just saying. The pid namespace (pid_t) may well have stricter limits, and maybe the compiler knew about them.

marxin commented 7 years ago

But the pid_t is int on x86_64 which is 32-bit long integer. Thus the warning message should be fine. Well, for compiler it's just:

typedef int __pid_t; typedef __pid_t pid_t;