Skip to content
Snippets Groups Projects

config: add is-modified helper

Open config: add is-modified helper
jnqnfe/vlc:is_modified into master
4 unresolved threads
Open Lyndon Brown requested to merge jnqnfe/vlc:is_modified into master
4 unresolved threads

To be used by a new 'expert' prefs mode (!1344 (merged)).

Merge request reports

Members who can merge are allowed to add commits.

Merge request pipeline #188738 passed

Stage:

    Merge request pipeline passed for 10afbf4b

    Approval is optional
    Merge blocked: 3 checks failed

    Merge details

    • The source branch is 15608 commits behind the target branch.
    • 1 commit will be added to master.
    • Source branch will be deleted.

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
  • Steve Lhomme
  • Steve Lhomme
  • Bart - The Review Bot added MRStatus::InReview label and removed MRStatus::Reviewable label

    added MRStatus::InReview label and removed MRStatus::Reviewable label

  • Lyndon Brown added 1 commit

    added 1 commit

    • d9124bce - config: add is-modified helper

    Compare with previous version

  • Lyndon Brown resolved all threads

    resolved all threads

  • Rémi Denis-Courmont
    Rémi Denis-Courmont @Courmisch started a thread on the diff
    • src/config/core.c
    464 464 return (param != NULL) ? &param->item : NULL;
    465 465 }
    466 466
    467 bool vlc_config_ItemIsModified(const module_config_t *item)
    468 {
    469 bool is_modified;
    470 switch (CONFIG_CLASS(item->i_type))
    471 {
    472 case CONFIG_ITEM_FLOAT:
    473 is_modified = (item->value.f != item->orig.f);
  • Rémi Denis-Courmont
    Rémi Denis-Courmont @Courmisch started a thread on the diff
    • src/config/core.c
    464 464 return (param != NULL) ? &param->item : NULL;
    465 465 }
    466 466
    467 bool vlc_config_ItemIsModified(const module_config_t *item)
    468 {
    469 bool is_modified;
    470 switch (CONFIG_CLASS(item->i_type))
    471 {
    472 case CONFIG_ITEM_FLOAT:
    473 is_modified = (item->value.f != item->orig.f);
    474 break;
    475 case CONFIG_ITEM_BOOL:
    476 case CONFIG_ITEM_INTEGER:
    477 is_modified = (item->value.i != item->orig.i);
  • Rémi Denis-Courmont
    Rémi Denis-Courmont @Courmisch started a thread on the diff
    • src/config/core.c
    473 is_modified = (item->value.f != item->orig.f);
    474 break;
    475 case CONFIG_ITEM_BOOL:
    476 case CONFIG_ITEM_INTEGER:
    477 is_modified = (item->value.i != item->orig.i);
    478 break;
    479 case CONFIG_ITEM_STRING:
    480 {
    481 bool orig_is_empty = (item->orig.psz == NULL || item->orig.psz[0] == '\0');
    482 bool curr_is_empty = (item->value.psz == NULL || item->value.psz[0] == '\0');
    483 if (orig_is_empty)
    484 is_modified = !curr_is_empty;
    485 else if (curr_is_empty)
    486 is_modified = true;
    487 else
    488 is_modified = (strcmp(item->value.psz, item->orig.psz) != 0);
    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      Even worse here as this can lead to use after free.

    • Lyndon Brown
      Lyndon Brown @jnqnfe ·
      Author Contributor

      I've made an attempt at further clarifying intentions in the documentation. (The function started off life as an inline helper in vlc_configuration.h; I lost sight of the greater need to document the locking aspect in moving it to avoid undesirable inclusion of vlc_plugin.h).

      The intention is for this function to only be used either when you already hold the config lock, or if there otherwise are no race issues because you otherwise have exclusive access to the value. The preference interfaces are given copies of the config items and create owned copies of values of string type, so have no need for locking/atomics in comparing orig and value. (Though I'm aware #26237 needs fixing for things to be completely safe).

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      Thread safety is not optional. Intention do not override requirements essentially imposed by the C specifications through the C compiler.

      What #26237 is, is a Qt bug. What it is not is an excuse to introduce a bug in the core code.

    • Lyndon Brown
      Lyndon Brown @jnqnfe ·
      Author Contributor

      Thread safety is not optional.

      Agreed, and I naturally have no wish to introduce thread safety bugs.

      What #26237 is... not is an excuse to introduce a bug in the core code.

      All I'm doing is adding a function for which the burden of ensuring thread safety is placed upon the caller rather than the function. This is fundamentally just a "you must hold the lock to use this function" type of situation, which isn't new and nor is it a bug.

      I'm not using #26237 as an excuse for anything; I only mentioned it because it felt wrong to make the simple claim that there is no thread safety issue without mentioning the complication #26237 has upon such a statement.

      What #26237 is, is a Qt bug.

      The source of the safety problems of the Qt module's handling of config values is not restricted to the Qt module itself, if there is even anything wrong within the module itself at all. The data set returned by module_config_get() is inherently unsafe. It gives out shallow copies of config items risking use-after-frees. There are a whole bunch of places in the wider codebase using config_PutPsz() which could invalidate the pointers placed into the dataset returned by module_config_get() before it's even finished building said dataset. Fixing this issue requires use of the lock and strdup() in module_config_get(). (Or alternatively the config lock should be exposed such that preference interfaces can duplicate the strings into owned QString/NSString form without the intermediate strdup()).

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      module_config_get() is inherently unsafe.

      It is an ugly hack though the code before it was even uglier. But I don't see how it is inherently unsafe.

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      (And even if it were inherently unsafe, there are no reasons to add another bug here.)

    • Lyndon Brown
      Lyndon Brown @jnqnfe ·
      Author Contributor

      It is an ugly hack though the code before it was even uglier. But I don't see how it is inherently unsafe.

      Indeed, I don't like it either. I don't recall if I've ever seen what came before.

      I consider it unsafe because module_config_get() is just sharing the item->value.psz pointers of string items with callers, which is racy with respect to any use of config_PutPsz() in other threads, and even more risky in general should a caller happen to not create owned copies of the value strings for themselves immediately after calling module_config_get().

      Use of config_PutPsz() occurs not only via the save button of the preference interface, but from things like the save button of the effects dialog; from 'set bookmark' hotkey actions; and from the podcast SD plugin (Run() -> ParseRequest() -> SaveUrls()). A use-after-free may be very unlikely to occur, but the risk is there.

      Fixing this, without replacing module_config_get(), obviously necessitates use of the lock and stdup() within module_config_get().

      (And even if it were inherently unsafe, there are no reasons to add another bug here.)

      The intention is not to just "add another bug" but rather to get the new expert prefs interface in place (which uses this function), ignoring the existing locking issue, and then explore fixing the locking issue (which should not involve touching this function) alongside addressing the issue of changes in each of the interfaces being entirely isolated from each other, which will involve to some extent reworking the entire underlying data model of the overall preferences interface.

      Of course applying the described fix to module_config_get() is actually trivial so could come first.

      Edited by Lyndon Brown
    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      You're conflating the known intrinsic bugginess of the value field, and alleged and still unexplained bugginess of module_conf_get() as a whole.

      I maintain my opposition to this MR.

    • Please register or sign in to reply
  • Lyndon Brown added 1 commit

    added 1 commit

    • fcca1901 - config: add is-modified helper

    Compare with previous version

  • Steve Lhomme
    Steve Lhomme @robUx4 started a thread on an old version of the diff
    • include/vlc_configuration.h
    251 251 */
    252 252 VLC_API module_config_t *config_FindConfig(const char *name) VLC_USED;
    253 253
    254 /**
    255 * Check item's modified state.
    256 *
    257 * This function checks whether or not an item's saved state is considered to
    258 * be default or modified. Currently we consider state to be modified only if
    259 * the value is different from it's default value.
    260 *
    261 * \warning The check is performed without locking. You must already hold the
    262 * config lock or otherwise have suitable exclusive access to the item's
    263 * attributes such as to prevent races.
    • Steve Lhomme
      Steve Lhomme @robUx4 ·
      Developer

      It seems the lock you're talking about is extern vlc_mutex_t config_lock; in the core. It's not exported and there's no public API to lock it. So either you do the lock in the core function, or you can't export vlc_config_ItemIsModified() because that lock can never be taken.

    • Lyndon Brown changed this line in version 4 of the diff

      changed this line in version 4 of the diff

    • Lyndon Brown
      Lyndon Brown @jnqnfe ·
      Author Contributor

      I'm only mentioning that lock in terms of future potential, for instance should this function get used somewhere within the core with the lock held.

      The second part of the sentence is what applies to the preference interfaces currently. As described in reply to Remi, the preference interfaces get their own local copy of the config items from the core. No locking is required for them to use this function with their local copy, so long as they have suitable exclusive access to the contents of that local copy. The copy given out by module_get_config() is currently a purely shallow copy, thus shares string pointers with the core's copy; Only the value string pointer is mutable through the lifetime of the program, and the preference interfaces must ensure that they have a strdup() copy of it if they are to safely do things such as make use of this function. The only current user of this function - the expert prefs interface - does this, though is still subject to the safety issue of #26237 currently affecting the existing advanced prefs interface, which needs addressing separately.

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      Alleged future potential is not a reason to knowingly introduce bugs.

      Either you substantiate how to "otherwise have suitable exclusive access", or you remove that mention. In the later case, I agree with @robUx4 that it makes no sense to export the function.

    • Lyndon Brown
      Lyndon Brown @jnqnfe ·
      Author Contributor

      How they achieve that is fundamentally interwoven with fixing #26237. As just mentioned in the other thread, the current output of module_get_config() is inherently unsafe. That function needs to use the config lock and perform a strdup() upon the value attribute (if of string type). I'm not aware off the top of my head that anything else will actually be necessary to fix that bug, and with that fix, the one user of this new function should naturally be using it safely, given, IIRC, that any editing of its local copy involves no thread safety concerns itself, and flushing of changes to the core goes through config_PutPsz() which uses locking.

    • Rémi Denis-Courmont
      Rémi Denis-Courmont @Courmisch ·
      Maintainer

      I disagree both with your premise and your conclusion.

    • Please register or sign in to reply
  • Lyndon Brown added 1 commit

    added 1 commit

    • dbb4974a - config: add is-modified helper

    Compare with previous version

  • Rémi Denis-Courmont added Component::Core label

    added Component::Core label

  • Lyndon Brown added 1 commit

    added 1 commit

    • 10afbf4b - config: add is-modified helper

    Compare with previous version

  • Bart - The Review Bot added MRStatus::Stale label and removed MRStatus::InReview label

    added MRStatus::Stale label and removed MRStatus::InReview label

  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

  • Bart - The Review Bot added MRStatus::NotCompliant label and removed MRStatus::Stale label

    added MRStatus::NotCompliant label and removed MRStatus::Stale label

    • Please register or sign in to reply

    VideoLAN code repository instance