[vlc-devel] [PATCH 1/5] macosx: fix potential null deref in prefs tree construction

[Marvin Scholz]

On 24 Sep 2020, at 1:09, Lyndon Brown wrote:

> From: Lyndon Brown <jnqnfe at gmail.com>
> Date: Thu, 25 Apr 2019 04:11:43 +0100
> Subject: macosx: fix potential null deref in prefs tree construction
>
> subCategoryItem would be null if:
>  a) a plugin author neglected to place a set_category() call before 
> one
>     or more options.
>  b) the first or only subcat targetted by a plugin is a general one
>

Hi, in Objective-C it is fine to send a message to a nil object,
it will just return nil as well. (Which allows for easy call chaining
without having to check for nil along the way)

> there are actual examples of A with a handful of in-tree plugins (to 
> be
> fixed in a subsequent commit). i don't have a mac; did it not crash
> before this fix?
>
> there are multiple in-tree modules which target general subcats,
> including various logger and keystore plugins. again, did this not
> cause it to crash?
>
> diff --git a/modules/gui/macosx/preferences/prefs.m 
> b/modules/gui/macosx/preferences/prefs.m
> index f74afc9752..a8847e7189 100644
> --- a/modules/gui/macosx/preferences/prefs.m
> +++ b/modules/gui/macosx/preferences/prefs.m
> @@ -535,7 +535,7 @@
>                  }
>              }
>              else if (!module_is_main(p_module) && 
> (CONFIG_ITEM(configType) || configType == CONFIG_SECTION)) {
> -                if (![[subCategoryItem children] containsObject: 
> pluginItem]) {
> +                if (subCategoryItem && ![[subCategoryItem children] 
> containsObject: pluginItem]) {
>                      [[subCategoryItem children] 
> addObject:pluginItem];
>                  }
>

In this case though checking subCategoryItem seems important as the 
check would
wrongly return true if subCategoryItem is nil when evaluating

   ![[subCategoryItem children] containsObject: pluginItem]

it would not have too bad consequences as

   [[subCategoryItem children] addObject:pluginItem];

would be a no-op then (because subCategoryItem is nil)
but thats a bit ugly, as it makes it really hard to
understand what is going on in these cases, so IMO
the explicit check is fine, but the commit message should
be changed as it does not fix a null dereference.

>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel