| Message ID | 20210113221013.390592-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Common macros for QAPI list growth | expand |
14.01.2021 01:10, Eric Blake wrote: > On first glance, the loop in qmp_query_rx_filter() has early return > paths that could leak any allocation of filter_list from a previous > iteration. But on closer inspection, it is obvious that all of the > early exits are guarded by has_name, and that the bulk of the loop > body can be executed at most once if the user is filtering by name, > thus, any early exit coincides with an empty list. Add asserts to > make this obvious. A bit simpler (for me :) observation: But on closer inspection, it is obvious that all of the early exits are guarded by has_name, and in case when has_name is true we leave the loop (by break) immediately after allocation and assigning filter_list for the first time. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > net/net.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/net.c b/net/net.c > index e1035f21d183..e581c8a26868 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1211,6 +1211,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name, > if (nc->info->type != NET_CLIENT_DRIVER_NIC) { > if (has_name) { > error_setg(errp, "net client(%s) isn't a NIC", name); > + assert(!filter_list); > return NULL; > } > continue; > @@ -1236,6 +1237,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name, > } else if (has_name) { > error_setg(errp, "net client(%s) doesn't support" > " rx-filter querying", name); > + assert(!filter_list); > return NULL; > } >
On 1/13/21 10:54 PM, Vladimir Sementsov-Ogievskiy wrote: > 14.01.2021 01:10, Eric Blake wrote: >> On first glance, the loop in qmp_query_rx_filter() has early return >> paths that could leak any allocation of filter_list from a previous >> iteration. But on closer inspection, it is obvious that all of the >> early exits are guarded by has_name, and that the bulk of the loop >> body can be executed at most once if the user is filtering by name, >> thus, any early exit coincides with an empty list. Add asserts to >> make this obvious. > > A bit simpler (for me :) observation: > > But on closer inspection, it is obvious that all of the early exits are > guarded by has_name, and in case when has_name is true we leave the loop s/in case// > (by break) immediately after allocation and assigning filter_list for > the first time. Replacing my wording with this touched-up sentence is fine with me, if Markus would like to tweak the queued commit to incorporate it.
diff --git a/net/net.c b/net/net.c index e1035f21d183..e581c8a26868 100644 --- a/net/net.c +++ b/net/net.c @@ -1211,6 +1211,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name, if (nc->info->type != NET_CLIENT_DRIVER_NIC) { if (has_name) { error_setg(errp, "net client(%s) isn't a NIC", name); + assert(!filter_list); return NULL; } continue; @@ -1236,6 +1237,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name, } else if (has_name) { error_setg(errp, "net client(%s) doesn't support" " rx-filter querying", name); + assert(!filter_list); return NULL; }