Potential bug in extension management

Discuss requests for comments/changes posted in the Issue Tracker for the development of phpBB. Upcoming releases are 3.2/Rhea and 3.3.
User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Potential bug in extension management

Post by javiexin » Fri Jan 13, 2017 12:17 pm

Hi,

I have found what I believe might be a bug in Extension Management, but it is not clear to me what is the best approach to solve it. I am all for implementing whatever suggestion comes out of here, just need guidance on what you believe is the best approach.

The situation: if an extension is ENABLED, and then the extension folder is removed, or renamed, or otherwise altered (changed composer.json...), the Extension Manager does not act properly, in my view: in the Extension List, this extension appears as "INVALID", however, if you programmatically ask for the status, this is listed as ENABLED (it is listed by the all_enabled method, and a query to is_enabled returns true). Similarly, if a DISABLED extension is altered in a similar way, the extension is listed as INVALID in the Extension List as well, but it is part of the all_disabled and is_disabled methods... In both cases, the extension will NOT be in all_available, and is_available will return FALSE (after the fix that has recently been applied, as the checks were much more strict in one that in the other - see below for reference).

This should be somehow fixed, but I am not clear as how: if the "invalid" extensions are removed from the "ext" table, then they will be "unavailable", but the enable method and maybe the disable method have been executed, but not the delete_data method, therefore keeping the DB potentially not clean. The uninstall process cannot be performed reliably, as the migrations might not be available.

What do you think is the best approach? In reality, the is_enabled and is_disabled methods I think they should reply as false, as even the DB states that these extensions are configured, they are not even available. However, it might be ok that they stay as is, but in that case, it should be clearly documented that to check for an extension, it should be BOTH is_enabled AND is_available, and that the former does NOT guarantee the later.

Opinions?
Thanks,
-javiexin

EDIT: The mentioned "fix" to is_available is here: https://github.com/phpbb/phpbb/pull/4592

PS: Another question: is there a way to have an "enabled" extension to be "dormant", in the sense of NOT executing its modifications, without changing status? This would allow that "invalid" extensions could be put in this special state, and hence they would not interfere with the normal board functioning while the issue is fixed.

User avatar
3Di
Registered User
Posts: 695
Joined: Tue Nov 01, 2005 9:50 pm
Location: Milano (I) Frankfurt (D)
Contact:

Re: Potential bug in extension management

Post by 3Di » Fri Jan 13, 2017 9:33 pm

A clear and visual example of what Javier is talking about it's located here:
https://www.phpbb.com/community/viewtop ... &t=2402246
Want to compensate me for my interest? Don't ask me, just do it
I'm available for custom phpBB jobs, you can PM me the details.
New: my Live Board, Extensions and Tools for phpBB - Men at works.

User avatar
DavidIQ
Customisations Team Leader
Customisations Team Leader
Posts: 1732
Joined: Thu Mar 02, 2006 4:29 pm
Location: Earth
Contact:

Re: Potential bug in extension management

Post by DavidIQ » Sat Jan 14, 2017 3:28 am

I think that for the actual issue javiexin is talking about that the likely solution would have to be that is_enabled should return true only if is_available also returns true, which appears to be in the mentioned pull request.

I highly doubt any of this would prevent the error reported and caused by that user though. phpBB already tries to get around it, but if the code is used by that specific area of the ACP there's little that the system could do. Plus making the system work after a user deliberately deletes system files is not something that really needs to be handled nor can it be handled very well in all cases. It's just like if a Windows user deleted some files from the Windows/System32 folder causing the system and programs to malfunction. That's hardly the fault of the operating system and entirely the user's fault. In such a case the recommendation from most technicians would be to reinstall Windows or the affected software. In our case it would be to try and reinstall the extension or, in extreme cases, the entire forum.
Image

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Potential bug in extension management

Post by javiexin » Sat Jan 14, 2017 11:21 am

DavidIQ wrote:
Sat Jan 14, 2017 3:28 am
I think that for the actual issue javiexin is talking about that the likely solution would have to be that is_enabled should return true only if is_available also returns true, which appears to be in the mentioned pull request.
Not quite: the mentioned PR only makes is_available consistent with all_available. But there is no change to is_enabled.

From what I have seen, is_configured relies EXCLUSIVELY in the DB situation, if an entry in the "ext" table exists, then the extension is configured (ie, either enabled or disabled, no other option). However, is_available and all_available (after the change in the above PR) rely ONLY on the File System status, specifically in the composer.json file, and nothing else. So, the possibility to have "configured" extensions that are "not available" is there.

I would propose the following: when loading the extension cache, the availability is checked, and if the extension is not available, it will be removed from the configured list (either enabled or disabled), but will be maintained in the database; if later the extension is available again, I think it should NOT keep the "enabled" status as is, it should be "reenabled" (or whatever term you prefer). For that, the DB should have been updated to something DIFFERENT from "enabled" or "disabled", something "in between": equivalent in behaviour to disabled, but the "disable" method has not been run...

I will create a PR with the first proposed change, and see what you think, and what the tests tell us.
DavidIQ wrote:
Sat Jan 14, 2017 3:28 am
I highly doubt any of this would prevent the error reported and caused by that user though. phpBB already tries to get around it, but if the code is used by that specific area of the ACP there's little that the system could do. Plus making the system work after a user deliberately deletes system files is not something that really needs to be handled nor can it be handled very well in all cases. It's just like if a Windows user deleted some files from the Windows/System32 folder causing the system and programs to malfunction. That's hardly the fault of the operating system and entirely the user's fault. In such a case the recommendation from most technicians would be to reinstall Windows or the affected software. In our case it would be to try and reinstall the extension or, in extreme cases, the entire forum.
I am not so sure: I think that the error (blank screen on ACP access) is probably created by a "module" that is configured/enabled, and yet, not in the file system. This would likely be "solved" if the mentioned extension would be treated as disabled. It will keep a "stalled" entry in the DB, but at least, it will not make the board to malfunction.

On another related issue: the DB does NOT keep track of the extension VERSION that has been configured. In case the version is changed in the file system, this will stay unnoticed, and the extension might start to function improperly (depending on the changes from version to version). I think that this information should be kept in the DB, and if a mismatch is produced, the extension should also be "deactivated", and be forced to be reenabled...

Thanks...
-javiexin

EDIT: Ticket is now open https://tracker.phpbb.com/browse/PHPBB3-15009

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Potential bug in extension management

Post by javiexin » Sat Jan 14, 2017 12:34 pm

I have now created the PR (WIP status): https://github.com/phpbb/phpbb/pull/4644

Current implementation is PARTIAL. Only eliminates the "not available" extension from the "configured" list.

In order for this to work as expected, the code from https://github.com/phpbb/phpbb/pull/4592 is required, so I duplicated the changes here. Note that these changes are DUPLICATE, so they should not be merged twice :).

Limitations of current implementation:
  • cache is "almost" disabled (question: what is the best strategy to take advantage of cache while keeping consistency?)
  • the "configured" but not "available" extensions do not show up in the ACP Extension List (question: how/where to show them?)
  • no reenable mechanism, extension will "automatically" appear when it is available again in the filesystem
  • no version validation of "configured" vs "available" extensions
Anything else? Regards,
-javiexin

User avatar
DavidIQ
Customisations Team Leader
Customisations Team Leader
Posts: 1732
Joined: Thu Mar 02, 2006 4:29 pm
Location: Earth
Contact:

Re: Potential bug in extension management

Post by DavidIQ » Sat Jan 14, 2017 12:50 pm

So at the expense of performance we are going to try and get around a user error? This is probably why it is how it is now. Maybe an obvious solution is to update the extension table entry to not be enabled if it is invalid.
Image

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Potential bug in extension management

Post by javiexin » Sat Jan 14, 2017 12:55 pm

But then, the migrations have run, and the database might be corrupt, and for sure not consistent.

User avatar
DavidIQ
Customisations Team Leader
Customisations Team Leader
Posts: 1732
Joined: Thu Mar 02, 2006 4:29 pm
Location: Earth
Contact:

Re: Potential bug in extension management

Post by DavidIQ » Sat Jan 14, 2017 1:00 pm

If there's no recourse without sacrificing system performance then we do nothing. This is completely the fault of the user and not something we should be going out of our way to get around by making the system slow. It should be considered as if they deleted a system file.
Image

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Potential bug in extension management

Post by javiexin » Sat Jan 14, 2017 2:25 pm

Ok, the issue now being performance, what alternatives do we have? We could set a TTL on the cache (currently, no TTL is set as far as I can tell), and only on cache expiration this will be checked. There would be certain exposition, but not much (a max of TTL, so depends on that), and the performance would be improved by not accessing the file system (or the db) at every call, just the cache.

If a discrepancy is detected, the extension could be put in a "dormant" state: from an execution standpoint, identical to "disabled", but from a configuration standpoint, the "disable" method for the extension would have not been run, which might create issues (notifications is one that comes to mind here, similarly for new profile fields types, maybe others as well). This could still be problematic, as it might break the forum, but as David said, this would be probably too much to consider.

So the question becomes: when is a good time to check the file system for this kind of mismatches? How often? Performance vs reliability...
-javiexin

User avatar
VSE
Extension Customisations
Extension Customisations
Posts: 670
Joined: Mon Mar 08, 2010 9:18 am

Re: Potential bug in extension management

Post by VSE » Sat Jan 14, 2017 6:19 pm

David is right. People should not delete active extension files from the filesystem, just as doing so to phpBB's core files would also result in unexpected results.
Has an irascible disposition.

Post Reply