[MAGNOLIA-5730] Recursive groups causes an infinite loop with stackoverflow error in MgnlUser Created: 26/Mar/14 Updated: 06/May/14 Resolved: 27/Mar/14 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | core |
| Affects Version/s: | 4.5.16 |
| Fix Version/s: | 4.5.18, 5.2.4 |
| Type: | Bug | Priority: | Blocker |
| Reporter: | Edgar Vonk | Assignee: | Milan Divilek |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
| Template: |
|
| Patch included: |
Yes
|
| Acceptance criteria: |
Empty
|
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
| Release notes required: |
Yes
|
| Date of First Response: | |
| Visible to: |
Gordon Bentvelzen
|
| Description |
|
It seems we have discovered quite a serious bug related to the handling of user groups. When there are recursive groups in Magnolia the MgnlUser#addSubgroups method ends up in a recursive loop resulting in a stackoverflowerror. The method's JavaDoc claims to prevent against such infinite loops, however this is unfortunately not the case. The fix seems extremely simple. Just replace this code:
// and recursively add more subgroups
addSubgroups(allGroups, man, subgroups);
allGroups.addAll(subgroups);
with:
allGroups.add(groupName);
// and recursively add more subgroups
addSubgroups(allGroups, man, subgroups);
Because in the current code the 'allGroups' collection remains empty and no checking against recursive groups is done at all. In our temporary hotfix we have added a check to see if there actually are any subgroups: allGroups.add(groupName); if (!subgroups.isEmpty()) { // and recursively add more subgroups addSubgroups(allGroups, man, subgroups); } I have attached our MgnlUser.java with our fix (note: tabs instead of spaces so please reformat according to your conventions). It was build against Magnolia 4.5.16 but I think the issue is present in all recent Magnolia versions. Our situation: our client has a lot of groups in Magnolia and in some cases we have a recursive situation where a subgroup contains a parent group somewhere in the tree. I reproduced this bug locally with a very simple test case using 2 groups where group A contains group B as a subgroup and group B again contains group A. The current unit tests of the MgnlUser class do not contain such a recursive group setup. They should. |
| Comments |
| Comment by Edgar Vonk [ 26/Mar/14 ] |
|
Forgot to mention: the issue occurs on many situations but the most prominent is when the user in question logs in to Magnolia. In our case we have a custom group manager class WstGroupManager as you see in the stacktrace but this has nothing to do with the issue. The problem is in MgnlUser itself. 2014-03-26 13:30:21,767 ERROR info.magnolia.cms.security.SecuritySupportBase : Can't login due to: javax.security.auth.login.LoginException: java.lang.StackOverflowError at org.apache.log4j.spi.LoggingEvent.<init>(LoggingEvent.java:165) at org.apache.log4j.Category.forcedLog(Category.java:391) at org.apache.log4j.Category.log(Category.java:856) at org.slf4j.impl.Log4jLoggerAdapter.error(Log4jLoggerAdapter.java:576) at info.magnolia.cms.security.SilentSessionOp.exec(SilentSessionOp.java:73) at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:403) at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:376) at com.worldsteel.magnolia.security.WstGroupManager.getGroup(WstGroupManager.java:110) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:372) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) at info.magnolia.cms.security.MgnlUser.addSubgroups(MgnlUser.java:379) [..] |
| Comment by Jan Haderka [ 27/Mar/14 ] |
|
what is the reason for removing serial version UID? This class implements Serializable and indeed is serialised when user session is serialised since we keep it in session via subject. Deserializing such session after update would cause exception since uid would no longer match. Plus it looks completely irrelevant change to the current issue. |
| Comment by Milan Divilek [ 27/Mar/14 ] |
|
serialVersionUID wasn't removed in this issue, but in |
| Comment by Jan Haderka [ 27/Mar/14 ] |
|
sry, i don't know why git it popped up in git code review tab as your change |