Jouni > On Oct 7, 2016, at 5:07 PM, Jouni Malinen <j@xxxxx> wrote: > > On Fri, Oct 07, 2016 at 04:28:06PM -0500, Joel Cunningham wrote: >> I’ve run into an issue with hostap running on an embedded platform. The qsort function from the standard C library data aborts when the list size is 0. We’ve seen this happen in wpa_supplicant_get_scan_results() with a zero-sized scan results. A zero-sized can results is probably not common, but this could be encountered with a device in an isolation chamber. > > Can you identify the C library used here? It does not sound like it > would be standards compliant if it asserts on the second argument to > qsort() being 0. That is supposed to result in no calls to the > comparison function and no rearrangement.. This particular embedded platform is quite old. It’s the ARM RVCT compiler, version 2.2 (ARM is now on version 5) which includes a standard C library. I did not find any ARM specific documentation on qsort that would be helpful in this case. > >> I’m wondering if the preferred solution is to introduce os_qsort in to the os.h abstraction layer that way I can add extra checks on the size before calling qsort. I’m kind of surprised there isn’t an abstracted call already since qsort is typically implemented in the standard C library and would fall under the OS_N_C_LIB_DEFINE feature. > > This is the first time I've seen a report pointing out issues with > qsort().. To be honest, I'd rather have that C library fixed than add > workarounds in wpa_supplicant unless someone can point to a C standard > that claims the behavior in that library implementation being allowed. > > There is obviously no need to call qsort() if there is less than two > entries in the array, so it would be trivial to skip those calls. I'm > not sure using an os_qsort() wrapper would be the best approach here, > though. > I forgot to mention another detail, in the case of wpa_supplicant_get_scan_results, the base pointer into qsort is NULL in addition to num being 0. This can be seen in the driver_nl80211.c (possibly other drivers too, I’m not sure) where nl80211_get_scan_results() calls os_zalloc() on struct wpa_scan_results and if no results are added to the res array in bss_info_handler, res->res and res->num will end up being NULL and 0. According to at least this reference, passing in a NULL pointer results in undefined behavior: http://www.cplusplus.com/reference/cstdlib/qsort/ In our local port of WPA supplicant, we added a check if res->num > 1 before calling qsort in wpa_supplicant_get_scan_results() and I’m fine with this approach as well (attached is a patch). I was leaning towards the os layer abstraction because it seemed that was the standard approach for interacting with standard C lib calls and it wasn’t clear why qsort wasn’t already in that abstraction. Plus using the abstraction saves adding a conditional to each location that uses qsort to ensure no other NULL pointers are being passed. > -- > Jouni Malinen PGP id EFC895FA > > _______________________________________________ > Hostap mailing list > Hostap@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/hostap Thanks, Joel
Attachment:
0001-WPA-supplicant-scan-check-scan-results-size-before-c.patch
Description: Binary data
_______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
