[Mesa-dev] [RFC PATCH] dri megadriver_stub: provide compatibility with older DRI loader

[Keith Packard]

Jordan Justen <jljusten at gmail.com> writes:

>> We find the driver foo's name by using the dladdr function
>> which gives the path of the dynamic library's name that
>> was being loaded.

That sounds like all kinds of win for existing X servers. Thanks for
doing it up in style, so that a megadrivers build can actually work for
all chips in the megadriver.

(as a style issue, I probably would have computed the driver name from
the dli_fname value directly and then copied it out into new storage, or
directly into get_extensions_name, but that's just me, so you should
feel free to ignore it :-)

>> +   /* Make sure the patch ends with _dri.so */
>> +   name_len = strlen(driver_name);
>> +   if (strcmp(driver_name + (name_len - 7), "_dri.so") != 0) {

Need to make sure name_len is >= 7 here. Should probably just have a
test that checks namelen and bails if it's < 7 as there are other places
using this magic value.

(Oh, I'd probably stick "_dri.so" in a #define and then #define the
length of it too, instead of using '7' in several places. Again, style,
not substance, so you can ignore that as you please.)

>> +   i = asprintf(&get_extensions_name, "%s_%s",
>> +                __DRI_DRIVER_GET_EXTENSIONS, driver_name);
>> +   free(driver_path);
>> +   if (i == -1 || !get_extensions_name)
>> +      return;

Is the null pointer check here useful or necessary? asprintf doesn't
define the value when allocation fails, preferring to return -1
instead. Are there systems which return valid 'i' and null pointer?

>> +   /* Copy the extensions into the __driDriverExtensions array
>> +    * we declared.
>> +    */
>> +   for (i = 0; i < ARRAY_SIZE(__driDriverExtensions); i++) {
>> +      __driDriverExtensions[i] = extensions[i];
>> +      if (extensions[i] == NULL)
>> +         break;
>> +   }
>> +
>> +   /* If the driver had more extensions that we reserved, then
>> +    * bail out. This will cause the driver to fail to load using
>> +    * the older loader mechanism.
>> +    */
>> +   if (extensions[i] != NULL) {

This check is incorrect -- you will dereference off the end of the array
when you fill it. Instead, you should just check for

        if (i == ARRAY_SIZE(__driDriverExtensions))

as that will let you know the array was filled

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131206/393f249d/attachment-0001.pgp>