Consolidate MAC-48 address matching code. #1507
Conversation
|
For this change I have prepared 5 more commits to get some details right, these may require minor clean-ups. If everything goes well, I am going to declare this lot ready for review next week. |
infrastation
force-pushed
the
gen_mac48host
branch
from
cc1cd3d to
5999ded
Compare
|
This is going to be merged later today unless anyone objects. |
|
The failure on |
Back to previous tcc. (I upgraded it on 16/04). |
|
That's not specific to the new TinyCC, please see this message. The next revision will not add |
Make the variables that need to be managed global, add a function to deallocate all resources if required and call it before exit() both in main() and in error(). This improves Valgrind SNR and resolves the following in particular: $ valgrind --leak-check=full --show-leak-kinds=all --quiet \ ./testprogs/filtertest EN10MB 'invalid syntax' filtertest: can't parse filter expression: syntax error ==411153== 15 bytes in 1 blocks are still reachable in loss record 1 of 2 ==411153== at 0x48417B4: malloc (vg_replace_malloc.c:381) ==411153== by 0x10EFCB: copy_argv (filtertest.c:229) ==411153== by 0x10EFCB: main (filtertest.c:419) ==411153== ==411153== 648 bytes in 1 blocks are still reachable in loss record 2 of 2 ==411153== at 0x48465EF: calloc (vg_replace_malloc.c:1328) ==411153== by 0x117651: pcap_open_dead_with_tstamp_precision (pcap.c:4476) ==411153== by 0x10EE7B: main (filtertest.c:392) ==411153==
pcap_ether_hostton() returns an allocated 48-bit binary MAC address, which ought to be freed no matter if the attempt to use it was successful or not. The latter case stands for several code paths that start at gen_scode(), invoke pcap_ether_hostton() and eventually bpf_error(); each of these ought to arrange a means to free the memory. Some of these code paths are already covered with tests, add tests to cover the remaining code paths and make the problem space completely visible to Valgrind.
pcap_ether_hostton() returns a pointer to allocated memory, which needs to be freed, which gen_scode() does for the successful case only. To have the memory freed in any case, copy the value into a local variable and free the allocated memory immediately thereafter -- this suffices since the data size is fixed and the value needs to be used only once per code path. In other words, apply the solution from commit 2b01095 to the problem from commit bcbef22.
gen_gateway(), gen_scode(), gen_ecode() and gen_broadcast() all have a switch block with 11 or 13 DLTs and a default case. The purpose of the latter is to avoid any calls to pcap_ether_hostton() in gen_scode() case Q_HOST proto Q_LINK if the DLT does not use MAC-48 addresses, so add a new is_mac48_linktype() function to support this behaviour; for consistency add the same guard to gen_scode() case Q_GATEWAY and to gen_ecode(). The purpose of the latter is to avoid any calls to pcap_ether_hostton() in gen_scode() if the DLT does not use MAC-48 addresses, so add a new is_mac48_linktype() function to support this behaviour; add similar guards to gen_scode() case Q_GATEWAY and to gen_ecode() for consistency. These changes replace a few custom messages with templates, so update, add and coalesce the tests as required.
infrastation
force-pushed
the
gen_mac48host
branch
from
5999ded to
8f99c57
Compare
|
This is the next revision. As before, it does not change anything with regard to The "Plug memory leaks from pcap_ether_hostton()." commit is meant to be cherry-picked into the stable branch, hence it changes code that the next commit replaces. Please proof-read if you can, I am going to merge this soon in order to unblock #1504. |
This is the less straightforward change I mentioned in #1505 earlier.