ubsan fixes for various tcpdump printers

[fenner]

I ran some fuzzing with ubsan enabled. These are my fixes to the problems that this fuzzing found.

(This was kind of an accident; I didn't realize that ubsan was enabled and I got a bunch of surprise fuzz failures, but I figure these fixes are probably helpful anyway)

[infrastation]

Thank you for preparing these changes. Do you still have the UBSan messages? Does make check pass in your working copy?

[fenner]

Thank you for preparing these changes. Do you still have the UBSan messages? Does make check pass in your working copy?

I will recreate the UBSan messages, by removing the patches and rebuilding and using the test pcaps. I didn't keep records when I did this work, only pcaps. I'll put the corresponding messages in with each commit that fixes it?

The out files were generated with 4.99.1, so if there are changes in master that could explain the mismatch. I will regenerate them.

[fenner]

P.S. I will also squash the "Address integer overflows" commit into the "ISO:" commit once my discussion with fxlb is complete.

[infrastation]

Placing the messages into respective commits would be perfect, thank you!

[fenner]

The integer overflow commit is still separate, because I want to validate it against a separate implementation of this checksum implementation that I have access to.

I updated the .out files to pass with master in this commit, but I see illumos has already failed so I will investigate the check failure(s) and update.

[infrastation]

Rebased on the current master and added a fixup, the expectation is that this pull request passes all CI checks.

[fenner]

I didn't think it was worth pushing my fix for the illumos build, but I have done so now to avoid getting confused by

Your branch and 'origin/fenner-ubsan-fixes' have diverged,
and have 6 and 14 different commits each, respectively.

What's pending here is extracting the proprietary iso checksum algorithm into a form that I can test with. Once I have validated that our checksum algorithm and tcpdump's have the same result, I will squash the checksum fixes into a single commit, push that and at that point I think it will be ready to merge.

[infrastation]

Alright, thank you for the clarifications.

[fenner]

I apologize for the delay. In order to double-check that the new checksum algorithm gets the right value, I have processed all of the pcap files that @fxlb provided using this main.c, and both the (proprietary) gated and the updated tcpdump algorithm got the same value for each packet.

#include <pcap/pcap.h>

void callback(u_char *user, const struct pcap_pkthdr *h,
                                                const u_char *bytes) {
   int gated_cksum, tcpdump_cksum;
   gated_cksum = gated_iso_cksum( bytes, h->caplen, bytes );
   tcpdump_cksum = create_osi_cksum( bytes, 0, h->caplen );
   if ( gated_cksum != tcpdump_cksum ) {
      fprintf( stderr, "*** NOT MATCHING *** " );
   }
   fprintf( stderr, "gated %04x tcpdump %04x\n", gated_cksum, tcpdump_cksum );
}

int main(int argc, const char **argv) {
   char errbuf[PCAP_ERRBUF_SIZE];
   pcap_t *p;
   p = pcap_open_offline( argv[ 1 ], errbuf );
   pcap_loop( p, -1, callback, NULL );
}

[fxlb]

I have commited the fix for SNMP. I tried to test the fix for lwres (with/without) without success. Removing the patch, I don't get "addition of unsigned offset to xx be overflowed to yy" messages. What are OS/compiler/configure options for this one?

[fenner]

What are OS/compiler/configure options for this one?

It's likely that some of these are specific to 32-bit builds (e.g., i386_el7).

[fxlb]

What are OS/compiler/configure options for this one?

It's likely that some of these are specific to 32-bit builds (e.g., i386_el7).

You are rigth. I need clang, sanitize undefined + address and 32-bit mode. With it I reproduce the first error in print-lwres.c, line 294. Was the error on line 549 obtained with the same pcap?

[fenner]

Was the error on line 549 obtained with the same pcap?

I'm sorry, I did this work over a year ago and did not expect to have to provide detailed notes. I committed all of my work to the Arista repository, and this is the only lwres pcap that I have in the Arista repository, so my best guess is that it was.

[fxlb]

I understand. Can you run the program again with the lwres pcap to check if it displays one or two error messages?

[fenner]

I understand. Can you run the program again with the lwres pcap to check if it displays one or two error messages?

I tried again (with a tcpdump-4.99.2 base) and only get the one:

print-lwres.c:294:10: runtime error: addition of unsigned offset to 0xf40032be overflowed to 0x96a2d560
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-lwres.c:294:10 in 

[fxlb]

Same for tcpdump-4.99.3, tcpdump-4.99.4, master. Thank you! I have commited the fix for lwres with, in commit message, the error on line 294.