Warnings when building with "enable remote packet capture" (gcc)

[fxlb]

./pcap-new.c: In function 'pcap_open':
./pcap-new.c:453:39: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 256 [-Wformat-truncation=]
  453 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s (%s)",
      |                                       ^~
  454 |       name, pcap_statustostr(status), fp->errbuf);
      |       ~~~~                             
./pcap-new.c:453:3: note: 'snprintf' output 6 or more bytes (assuming 1285) into a destination of size 256
  453 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s (%s)",
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  454 |       name, pcap_statustostr(status), fp->errbuf);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./pcap-new.c:456:39: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 256 [-Wformat-truncation=]
  456 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s",
      |                                       ^~
  457 |       name, pcap_statustostr(status));
      |       ~~~~                             
./pcap-new.c:456:3: note: 'snprintf' output 3 or more bytes (assuming 1026) into a destination of size 256
  456 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s",
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  457 |       name, pcap_statustostr(status));
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./pcap-new.c:448:39: warning: '%s' directive output may be truncated writing up to 1023 bytes into a region of size 256 [-Wformat-truncation=]
  448 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s",
      |                                       ^~
  449 |       name, fp->errbuf);
      |       ~~~~                             
./pcap-new.c:448:3: note: 'snprintf' output between 3 and 1282 bytes into a destination of size 256
  448 |   snprintf(errbuf, PCAP_ERRBUF_SIZE, "%s: %s",
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  449 |       name, fp->errbuf);
      |       ~~~~~~~~~~~~~~~~~
./pcap-rpcap.c: In function 'rpcap_process_msg_header':
./pcap-rpcap.c:3354:89: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 214 [-Wformat-truncation=]
 3354 |    snprintf(remote_errbuf, PCAP_ERRBUF_SIZE, "Read of error message from client failed: %s", errbuf);
      |                                                                                         ^~   ~~~~~~
./pcap-rpcap.c:3354:4: note: 'snprintf' output between 43 and 298 bytes into a destination of size 256
 3354 |    snprintf(remote_errbuf, PCAP_ERRBUF_SIZE, "Read of error message from client failed: %s", errbuf);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[guyharris]

The current API limits what can be done here - some calls take, as an argument, a pointer to a buffer into which an error message is placed, and that buffer is limited to PCAP_ERRBUF_SIZE bytes.

The routines that don't take such a buffer, because pcap_geterr() is used to fetch the error, don't have this problems, although there might be code that assumes that the result of pcap_geterr() is limited to PCAP_ERRBUF_SIZE bytes in length.

We could:

• have, in a pcap_t, both the existing PCAP_ERRBUF_SIZE + 1-byte sized buffer and a pointer to an arbitrary-length buffer, have all the internal calls that construct an error message free the existing message pointed to by that pointer and use asprintf() to construct a new message;
• have a pcap_errstring() or whatever routine, which we explicitly do not guarantee to limit the length of the error message, and whose return value we do not guarantee will continue to be valid after any subsequent pcap API call, that returns the pointer to the arbitrary-length buffer;
• have pcap_geterr() safely truncate that arbitrary-length message (so that we don't have part of a UTF-8 sequence at the end), putting the result in the existing buffer, and returning a pointer to that buffer;

and then, if we come up with replacements for the routines that explicitly take an error buffer pointer, have the replacements take a char ** argument that's filled in with a pointer to an allocated error message, with the caller required to free it when they're done (using another pcap API to free it, in order to deal with the "WinPcap/Npcap was compiled with one C compiler and the application was compiled with another C compiler, so there are two different malloc/free arenas" problem on Windows).

[guyharris]

For some unknown reason, gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 only complains about the snprintf() call in pcap_open_live() if we're building with remote capture enabled. The compiler does not give its rationale for concluding that the message might be truncated, so I'm not sure why it believes it only if remote capture is enabled (that might well be true due to, for example, getting longer error messages if a remote device is opened, as the error message comes from the server and the compiler cannot determine the maximum length by reading the code, but is the compiler doing library-wide interprocedural analysis?).

[fxlb]

The reported warnings were given by: gcc (Debian 10.2.1-6), on Testing/Bullseye.

[fxlb]

I don't know enough the code affected by these changes. So I can't give a relevant opinion. I let you choose the best solution.

[fxlb]

For testing build with -Werror and see the remaining warnings, I have committed in my WIP PR #1028 a workaround with fxlb@a2776c5. Should I commit this while waiting for a solution?

[guyharris]

Should I commit this while waiting for a solution?

Yes.

[fxlb]

Workaround :
Commit 8e0543e in master.
Commit b59e572 in libpcap-1.10.

[infrastation]

Commit 05068a7 implements a different solution to the same problem.

[infrastation]

So does the trim buffer seem a better long-term workaround than disabling the warning?

[fxlb]

I don't know. I don't get the warnings when reverting your commit on Debian Bullseye with gcc 10.2.1-6 nor clang version 11.0.1-2.

[infrastation]

I could not find any Linux-specific examples, but on OpenBSD it manifested as follows: https://ci.tcpdump.org/#/builders/31/builds/147

[infrastation]

This looks resolved, any objections to closing?

[infrastation]

Closing then.