os layer qsort

[Joel Cunningham]

 On Oct 09, 2016, at 03:20 AM, Jouni Malinen <j at w1.fi> wrote:

> On Sat, Oct 08, 2016 at 12:39:24PM -0500, Joel Cunningham wrote:
>> 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.
> 
> Are you sure the C library asserts on the array length 0 instead of that
> NULL base pointer?

You're correct, the abort happens because of the NULL base pointer.  Sorry for not clearly communicating that to begin with.  We ended up using a check against the size instead of base pointer to also skip the qsort function call when num is 1.

> 
> 
>> 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/
> 
> I'd be more supportive of checking that base != NULL before the qsort()
> calls.. In fact, that's already done in almost all the other locations..
> 
>> 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.
> 
> Only the calls in wpa_supplicant_get_scan_results() and
> hostapd_config_read_maclist() seem to be able to pass in base == NULL to
> qsort(), so it is not really that many locations that would need
> changes.. I'd be fine with a patch adding such explicit checks if that
> fixes the issue you are seeing.

I have attached a patch containing NULL base pointer checks for these cases

> 
> The os_*() wrappers are for cases where there is no consistent behavior
> in C compilers/libraries. I'd rather not add more of them unless there
> is clear justification for the need.
> 
> -- 
> Jouni Malinen PGP id EFC895FA
> 
> _______________________________________________
> Hostap mailing list
> Hostap at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Check-for-NULL-qsort-base-pointers.patch
Type: application/octet-stream
Size: 1575 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/hostap/attachments/20161011/64f5f0fe/attachment.obj>