From a4caeb1e5d79a472bccb13545560534d620b9a2c Mon Sep 17 00:00:00 2001 From: Derrick Pallas Date: Wed, 14 Nov 2018 11:36:57 -0800 Subject: [PATCH 1/5] click/etherswitch: bail out on OOM Change-Id: Iedfff2d0be5fb8686a28fd8acb775c94f2fd7e63 Reviewed-on: https://gerrit.ikarem.io/74140 Static-Analysis: Jenkins Build System Tested-by: Jenkins Build System Reviewed-by: Peter Hurley Signed-off-by: Derrick Pallas --- elements/etherswitch/etherswitch.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/elements/etherswitch/etherswitch.cc b/elements/etherswitch/etherswitch.cc index 6f83f4f5ab..4983b405a8 100644 --- a/elements/etherswitch/etherswitch.cc +++ b/elements/etherswitch/etherswitch.cc @@ -62,7 +62,8 @@ EtherSwitch::broadcast(int source, Packet *p) for (int i = 0; i < n && w > 0; i++) { if (pfr.bv[i]) { Packet *pp = (w > 1 ? p->clone() : p); - output(i).push(pp); + if (pp) + output(i).push(pp); w--; } } From 1868fae509da1bc3ccd6858f429b30f6b83c06e9 Mon Sep 17 00:00:00 2001 From: Derrick Pallas Date: Wed, 14 Nov 2018 10:25:30 -0800 Subject: [PATCH 2/5] click/fromfile: plug mmap leak in error path If mmap is successful but Packet::make fails to create a new SKB, we need to execute the destructor we passed to Packet::make. It actually might make sense to do this in Packet::make instead instead of the caller, not sure which is better. Change-Id: If97d221daf3443003f40097cc36f7410bb544669 Reviewed-on: https://gerrit.ikarem.io/74137 Static-Analysis: Jenkins Build System Tested-by: Jenkins Build System Reviewed-by: Peter Hurley Signed-off-by: Derrick Pallas --- lib/fromfile.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/fromfile.cc b/lib/fromfile.cc index e0bb6dd24d..57217f13ad 100644 --- a/lib/fromfile.cc +++ b/lib/fromfile.cc @@ -157,6 +157,11 @@ FromFile::read_buffer_mmap(ErrorHandler *errh) return error(errh, "mmap: %s", strerror(errno)); _data_packet = Packet::make((unsigned char *)mmap_data, _len, munmap_destructor, 0); + if (!_data_packet) { + munmap_destructor((unsigned char *)mmap_data, _len, NULL); + return error(errh, "Packet::make failed"); + } + _buffer = _data_packet->data(); _file_offset = _mmap_off; _mmap_off += _len; From 7aec8b832b05e71a1999c700a0e9fe741cd589c3 Mon Sep 17 00:00:00 2001 From: Derrick Pallas Date: Wed, 14 Nov 2018 12:06:43 -0800 Subject: [PATCH 3/5] click/hashtable: assert that operator[] does not return a NULL reference There is no such thing as a NULL reference, so there is no way to tell if find_insert failed when invoked by operator[]. Some old code attempted to check !&x for the reference, but the compiler can optimize it out since it is "impossible" according to the ARM. Therefore, we should just assert that find_insert succeeds. Change-Id: I1d34e19033e77dd6aebfdba76d553e927711a62c Signed-off-by: Derrick Pallas --- include/click/hashtable.hh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/click/hashtable.hh b/include/click/hashtable.hh index 05814946c3..9de0cbfd0d 100644 --- a/include/click/hashtable.hh +++ b/include/click/hashtable.hh @@ -951,7 +951,9 @@ template typename HashTable::value_type & HashTable::operator[](key_const_reference key) { - return *find_insert(key); + HashTable_iterator i = find_insert(key); + assert(i.get()); + return *i; } template From f2a382109554074ef4abf8d482003bb099bd8dff Mon Sep 17 00:00:00 2001 From: Derrick Pallas Date: Wed, 14 Nov 2018 10:55:24 -0800 Subject: [PATCH 4/5] click/icmp: bail out on OOM Change-Id: Ia7b9f3c4a4e7547d17251edac67c75f967507b3f Reviewed-on: https://gerrit.ikarem.io/74139 Static-Analysis: Jenkins Build System Tested-by: Jenkins Build System Reviewed-by: Peter Hurley Signed-off-by: Derrick Pallas --- elements/icmp/icmpsendpings.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elements/icmp/icmpsendpings.cc b/elements/icmp/icmpsendpings.cc index 38eb592130..234fa12594 100644 --- a/elements/icmp/icmpsendpings.cc +++ b/elements/icmp/icmpsendpings.cc @@ -154,7 +154,7 @@ ICMPPingSource::make_packet(WritablePacket *q) q->set_ip_header(nip, sizeof(click_ip)); q->timestamp_anno().assign_now(); - if (_receiver) + if (p && _receiver) _receiver->send_timestamp[icp->icmp_sequence] = q->timestamp_anno(); return q; From 2191f9c852d42d1799f3cbf36e075412e39b49dd Mon Sep 17 00:00:00 2001 From: Tom Barbette Date: Thu, 15 Nov 2018 09:02:19 +0100 Subject: [PATCH 5/5] Fix f2a3821 --- elements/icmp/icmpsendpings.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elements/icmp/icmpsendpings.cc b/elements/icmp/icmpsendpings.cc index 234fa12594..8facdc7ff5 100644 --- a/elements/icmp/icmpsendpings.cc +++ b/elements/icmp/icmpsendpings.cc @@ -154,7 +154,7 @@ ICMPPingSource::make_packet(WritablePacket *q) q->set_ip_header(nip, sizeof(click_ip)); q->timestamp_anno().assign_now(); - if (p && _receiver) + if (q && _receiver) _receiver->send_timestamp[icp->icmp_sequence] = q->timestamp_anno(); return q;