| Message ID | 20150731115315.7fc0dc6dbe29df05af36197d@ubnt.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi, I have various issues with this patch On 31/07/2015 10:53, Dmitry Ivanov wrote: > Do not wait for scan results if scan request failed. > > Signed-off-by: Dmitry Ivanov <dima@ubnt.com> > --- > iwinfo_nl80211.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/iwinfo_nl80211.c b/iwinfo_nl80211.c > index 900eef2..251ec33 100644 > --- a/iwinfo_nl80211.c > +++ b/iwinfo_nl80211.c > @@ -389,11 +389,12 @@ static struct nl80211_msg_conveyor * nl80211_send( > while (err > 0) > nl_recvmsgs(nls->nl_sock, cv->cb); > > + if (err) > + goto err; > + this code will never be reached due tot he while (err > 0) above > return &rcv; > > err: > - nl_cb_put(cv->cb); > - nlmsg_free(cv->msg); > removing the freeeing code from a global helper is not a good idea and will cause a leak. John > return NULL; > } > @@ -2016,16 +2017,21 @@ static int nl80211_get_scanlist_cb(struct nl_msg *msg, void *arg) > > static int nl80211_get_scanlist_nl(const char *ifname, char *buf, int *len) > { > - struct nl80211_msg_conveyor *req; > + struct nl80211_msg_conveyor *req, *scan_res = NULL; > struct nl80211_scanlist sl = { .e = (struct iwinfo_scanlist_entry *)buf }; > > req = nl80211_msg(ifname, NL80211_CMD_TRIGGER_SCAN, 0); > if (req) > { > - nl80211_send(req, NULL, NULL); > + scan_res = nl80211_send(req, NULL, NULL); > nl80211_free(req); > } > > + if (!scan_res) > + { > + return -1; > + } > + > nl80211_wait("nl80211", "scan", NL80211_CMD_NEW_SCAN_RESULTS); > > req = nl80211_msg(ifname, NL80211_CMD_GET_SCAN, NLM_F_DUMP); >
Hi John! 1) The code is reachable because "err" is signed integer. Negative value is error indication. 2) Cleanup is called later in all cases. Actually, calling it here leads to an error - a kind of "double free". It was never encountered because it was never called in past. I had to create this patch after transition to the new kernel because AP scan patch was accidentally removed. Inability of kernel to do wifi scan in AP mode caused this situation where libiwinfo had to wait indefinitely for scan results. Ok, AP scan patch is restored now, but libiwinfo should be more robust against similar situations in future. In fact, I have tested my patch with both AP scan turned on and off. On Mon, Aug 17, 2015 at 1:18 PM, John Crispin <blogic@openwrt.org> wrote: > Hi, > > I have various issues with this patch > > On 31/07/2015 10:53, Dmitry Ivanov wrote: >> Do not wait for scan results if scan request failed. >> >> Signed-off-by: Dmitry Ivanov <dima@ubnt.com> >> --- >> iwinfo_nl80211.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/iwinfo_nl80211.c b/iwinfo_nl80211.c >> index 900eef2..251ec33 100644 >> --- a/iwinfo_nl80211.c >> +++ b/iwinfo_nl80211.c >> @@ -389,11 +389,12 @@ static struct nl80211_msg_conveyor * nl80211_send( >> while (err > 0) >> nl_recvmsgs(nls->nl_sock, cv->cb); >> >> + if (err) >> + goto err; >> + > > this code will never be reached due tot he while (err > 0) above > > >> return &rcv; >> >> err: >> - nl_cb_put(cv->cb); >> - nlmsg_free(cv->msg); >> > > removing the freeeing code from a global helper is not a good idea and > will cause a leak. > > > John > > > >> return NULL; >> } >> @@ -2016,16 +2017,21 @@ static int nl80211_get_scanlist_cb(struct nl_msg *msg, void *arg) >> >> static int nl80211_get_scanlist_nl(const char *ifname, char *buf, int *len) >> { >> - struct nl80211_msg_conveyor *req; >> + struct nl80211_msg_conveyor *req, *scan_res = NULL; >> struct nl80211_scanlist sl = { .e = (struct iwinfo_scanlist_entry *)buf }; >> >> req = nl80211_msg(ifname, NL80211_CMD_TRIGGER_SCAN, 0); >> if (req) >> { >> - nl80211_send(req, NULL, NULL); >> + scan_res = nl80211_send(req, NULL, NULL); >> nl80211_free(req); >> } >> >> + if (!scan_res) >> + { >> + return -1; >> + } >> + >> nl80211_wait("nl80211", "scan", NL80211_CMD_NEW_SCAN_RESULTS); >> >> req = nl80211_msg(ifname, NL80211_CMD_GET_SCAN, NLM_F_DUMP); >> > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On 20/08/2015 10:06, Dmitrijs Ivanovs wrote: > Hi John! > > 1) The code is reachable because "err" is signed integer. Negative > value is error indication. of course ... i read the while as "err will be 0 when it leaves the loop" > 2) Cleanup is called later in all cases. Actually, calling it here > leads to an error - a kind of "double free". It was never encountered > because it was never called in past. > ok, just looked at the patch without the context > I had to create this patch after transition to the new kernel because > AP scan patch was accidentally removed. Inability of kernel to do wifi > scan in AP mode caused this situation where libiwinfo had to wait > indefinitely for scan results. Ok, AP scan patch is restored now, but > libiwinfo should be more robust against similar situations in future. > In fact, I have tested my patch with both AP scan turned on and off. fair enough, will have a closer look later, thanks > On Mon, Aug 17, 2015 at 1:18 PM, John Crispin <blogic@openwrt.org> wrote: >> Hi, >> >> I have various issues with this patch >> >> On 31/07/2015 10:53, Dmitry Ivanov wrote: >>> Do not wait for scan results if scan request failed. >>> >>> Signed-off-by: Dmitry Ivanov <dima@ubnt.com> >>> --- >>> iwinfo_nl80211.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/iwinfo_nl80211.c b/iwinfo_nl80211.c >>> index 900eef2..251ec33 100644 >>> --- a/iwinfo_nl80211.c >>> +++ b/iwinfo_nl80211.c >>> @@ -389,11 +389,12 @@ static struct nl80211_msg_conveyor * nl80211_send( >>> while (err > 0) >>> nl_recvmsgs(nls->nl_sock, cv->cb); >>> >>> + if (err) >>> + goto err; >>> + >> >> this code will never be reached due tot he while (err > 0) above >> >> >>> return &rcv; >>> >>> err: >>> - nl_cb_put(cv->cb); >>> - nlmsg_free(cv->msg); >>> >> >> removing the freeeing code from a global helper is not a good idea and >> will cause a leak. >> >> >> John >> >> >> >>> return NULL; >>> } >>> @@ -2016,16 +2017,21 @@ static int nl80211_get_scanlist_cb(struct nl_msg *msg, void *arg) >>> >>> static int nl80211_get_scanlist_nl(const char *ifname, char *buf, int *len) >>> { >>> - struct nl80211_msg_conveyor *req; >>> + struct nl80211_msg_conveyor *req, *scan_res = NULL; >>> struct nl80211_scanlist sl = { .e = (struct iwinfo_scanlist_entry *)buf }; >>> >>> req = nl80211_msg(ifname, NL80211_CMD_TRIGGER_SCAN, 0); >>> if (req) >>> { >>> - nl80211_send(req, NULL, NULL); >>> + scan_res = nl80211_send(req, NULL, NULL); >>> nl80211_free(req); >>> } >>> >>> + if (!scan_res) >>> + { >>> + return -1; >>> + } >>> + >>> nl80211_wait("nl80211", "scan", NL80211_CMD_NEW_SCAN_RESULTS); >>> >>> req = nl80211_msg(ifname, NL80211_CMD_GET_SCAN, NLM_F_DUMP); >>> >> _______________________________________________ >> openwrt-devel mailing list >> openwrt-devel@lists.openwrt.org >> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >
diff --git a/iwinfo_nl80211.c b/iwinfo_nl80211.c index 900eef2..251ec33 100644 --- a/iwinfo_nl80211.c +++ b/iwinfo_nl80211.c @@ -389,11 +389,12 @@ static struct nl80211_msg_conveyor * nl80211_send( while (err > 0) nl_recvmsgs(nls->nl_sock, cv->cb); + if (err) + goto err; + return &rcv; err: - nl_cb_put(cv->cb); - nlmsg_free(cv->msg); return NULL; } @@ -2016,16 +2017,21 @@ static int nl80211_get_scanlist_cb(struct nl_msg *msg, void *arg) static int nl80211_get_scanlist_nl(const char *ifname, char *buf, int *len) { - struct nl80211_msg_conveyor *req; + struct nl80211_msg_conveyor *req, *scan_res = NULL; struct nl80211_scanlist sl = { .e = (struct iwinfo_scanlist_entry *)buf }; req = nl80211_msg(ifname, NL80211_CMD_TRIGGER_SCAN, 0); if (req) { - nl80211_send(req, NULL, NULL); + scan_res = nl80211_send(req, NULL, NULL); nl80211_free(req); } + if (!scan_res) + { + return -1; + } + nl80211_wait("nl80211", "scan", NL80211_CMD_NEW_SCAN_RESULTS); req = nl80211_msg(ifname, NL80211_CMD_GET_SCAN, NLM_F_DUMP);
Do not wait for scan results if scan request failed. Signed-off-by: Dmitry Ivanov <dima@ubnt.com> --- iwinfo_nl80211.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)