os layer qsort

[Joel Cunningham]

Jouni


> On Oct 7, 2016, at 5:07 PM, Jouni Malinen <j at w1.fi> 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap

Thanks,

Joel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-WPA-supplicant-scan-check-scan-results-size-before-c.patch
Type: application/octet-stream
Size: 1306 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/hostap/attachments/20161008/528c7770/attachment-0001.obj>