[Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Tom Stellard <thomas.stellard at amd.com> writes:
>
>> When probing for devices, clover will call pipe_loader_probe() twice.
>> The first time to retrieve the number of devices, and then second time
>> to retrieve the device structures.
>>
>> We currently assume that the return value of both calls will be the
>> same, but this will not be the case if a device happens to disappear
>> between the two calls.
>>
>
> Assuming that the two sequential calls will give the same result is bad,
> but the likelihood of a device disappearing between the two calls is so
> small it's unlikely to happen more than once in a million years.  If
> this problem happens deterministically to you there's almost certainly a
> more serious bug in pipe-loader that will cause it to return an
> incorrect device count during the first call.
>
> Indeed, pipe_loader_sw_probe() seems to return 1 unconditionally if ndev
> is zero, so it will give a different device count depending on ndev if
> pipe_loader_sw_probe_null() happens to fail.  Seems like a bug we don't
> want to simply paper over.
>
> Anyway I'm not saying that clover's assumption shouldn't be fixed too --
> See comment below.
>
>> This patch removes this assumption and checks the return value of the
>> second pipe_loader_probe() call to ensure it does not try to initialize
>> devices that no longer exits.
>>
>> CC: <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/state_trackers/clover/core/platform.cpp | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/core/platform.cpp b/src/gallium/state_trackers/clover/core/platform.cpp
>> index 328b71c..689d692 100644
>> --- a/src/gallium/state_trackers/clover/core/platform.cpp
>> +++ b/src/gallium/state_trackers/clover/core/platform.cpp
>> @@ -28,9 +28,10 @@ platform::platform() : adaptor_range(evals(), devs) {
>>     int n = pipe_loader_probe(NULL, 0);
>>     std::vector<pipe_loader_device *> ldevs(n);
>>  
>> -   pipe_loader_probe(&ldevs.front(), n);
>> +   n = pipe_loader_probe(&ldevs.front(), n);
>>  
>> -   for (pipe_loader_device *ldev : ldevs) {
>> +   for (int i = 0; i < n; ++i) {
>> +      pipe_loader_device *ldev = ldevs[i];
>
> There are many reasons to prefer range loops to induction variables
> (including iterators), and there's no need to change it here: Because
> ldevs is zero-initialized you can just wrap the push_back(create()) call
> around an 'if (ldev)' to make this a one-liner.
>
Let me be more explicit about what could go wrong with this change in
its current form: The return value of pipe_loader_probe() is the number
of devices available on the system, so basically the same race condition
is still possible if 'n' increases from the first call to the second,
and in that case you'll end up reading uninitialized memory past the end
of the array -- Worse than a null dereference.  Range loops make this
kind of situation impossible.

>>        try {
>>           devs.push_back(create<device>(*this, ldev));
>>        } catch (error &) {
>> -- 
>> 2.0.4
>>
>> _______________________________________________
>> mesa-stable mailing list
>> mesa-stable at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151128/67475fff/attachment-0001.sig>