Jetspeed 2
  1. Jetspeed 2
  2. JS2-473

Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1-dev
    • Fix Version/s: 2.1-dev, 2.1
    • Component/s: None
    • Labels:
      None

      Description

      While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it. Since they may get a copy, any modifications would be discarded. Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>. There is no reason to suppose the other uses are safe without looking into them further. I don't think I'm qualified to fix this without at least some guidance.

      Method
      getFragments():List of interface org.apache.jetspeed.om.page.Fragment
      Found usages (70 usages)
      Unclassified usage (70 usages)
      jetspeed-layouts (5 usages)
      org.apache.jetspeed.portlets.layout (5 usages)
      LayoutPortlet (2 usages)
      addPortletToPage(String, String) (1 usage)
      >>>> (279, 18) root.getFragments().add(fragment);
      removeFragment(String, String) (1 usage)
      >>>> (257, 18) root.getFragments().remove(f);
      MultiColumnPortlet (2 usages)
      doView(RenderRequest, RenderResponse) (1 usage)
      (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("
      ,") );
      processAction(ActionRequest, ActionResponse) (1 usage)
      (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("
      ,") );
      PageManagerLayoutEventListener (1 usage)
      handleEvent(LayoutEvent) (1 usage)
      >>>> (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
      jetspeed-page-manager (60 usages)
      org.apache.jetspeed.om.page (15 usages)
      ContentFragmentImpl.ContentFragmentList (1 usage)
      (441, 42) private List baseList = fragment.getFragments();
      TestPageObjectModel (14 usages)
      testFragmentManipulation() (14 usages)
      (108, 28) assertNotNull(root.getFragments());
      >>>> (114, 14) root.getFragments().add(frag1);
      >>>> (128, 15) frag2.getFragments().add(frag3);
      >>>> (129, 14) root.getFragments().add(frag2);
      (132, 25) assertTrue(root.getFragments().size()==2);
      (133, 27) Iterator i = root.getFragments().iterator();
      (142, 22) assertTrue(f.getFragments().size()==0);
      (147, 22) assertTrue(f.getFragments().size()==1);
      (149, 22) assertTrue(f.getFragments().size()==1);
      (150, 15) i = f.getFragments().iterator();
      >>>> (164, 11) f.getFragments().remove(frag3);
      >>>> (167, 11) f.getFragments().add(frag2);
      (168, 22) assertTrue(f.getFragments().size()==1);
      (169, 29) f = (FragmentImpl)f.getFragments().get(0);
      org.apache.jetspeed.om.page.psml (4 usages)
      PageImpl (4 usages)
      getFragmentById(String) (1 usage)
      (177, 28) Iterator i = f.getFragments().iterator();
      getFragmentsByName(String) (1 usage)
      (276, 28) Iterator i = f.getFragments().iterator();
      removeFragmentById(String) (2 usages)
      (209, 28) Iterator i = f.getFragments().iterator();
      >>>> (234, 28) if (parent.getFragments().remove(f))
      org.apache.jetspeed.page (41 usages)
      AbstractPageManager (2 usages)
      copyFragment(Fragment, String) (2 usages)
      (889, 37) Iterator fragments = source.getFragments().iterator();
      >>>> (894, 18) copy.getFragments().add(copiedFragment);
      PageManagerTestShared.Shared (15 usages)
      testSecurePageManager(TestCase, PageManager) (15 usages)
      >>>> (281, 34) root.getFragments().add(portlet);
      >>>> (287, 34) root.getFragments().add(portlet);
      (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
      (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
      (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
      (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
      (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
      (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
      (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
      (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
      (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
      (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
      (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
      (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
      (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
      TestCastorXmlPageManager (15 usages)
      testClonePage() (4 usages)
      (861, 30) List children = root.getFragments();
      (862, 40) List cloneChildren = cloneRoot.getFragments();
      (905, 26) assertNotNull(cf.getFragments());
      (906, 23) assertTrue(cf.getFragments().size() == 2);
      testCreatePage() (3 usages)
      >>>> (266, 14) root.getFragments().add(f);
      (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
      (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
      testGetPage() (3 usages)
      (190, 30) List children = root.getFragments();
      (237, 25) assertNotNull(f.getFragments());
      (238, 22) assertTrue(f.getFragments().size() == 2);
      testUpdatePage() (5 usages)
      (373, 28) assertNotNull(root.getFragments());
      (374, 30) assertEquals(1, root.getFragments().size());
      (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
      (394, 28) assertNotNull(root.getFragments());
      (395, 25) assertTrue(root.getFragments().isEmpty());
      TestDatabasePageManager (9 usages)
      testCreates() (2 usages)
      >>>> (317, 14) root.getFragments().add(portlet);
      >>>> (328, 14) root.getFragments().add(portlet);
      testGets() (4 usages)
      (620, 51) assertNotNull(check.getRootFragment().getFragments());
      (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
      (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
      (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
      testUpdates() (3 usages)
      (814, 46) assertNotNull(page.getRootFragment().getFragments());
      (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
      (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
      jetspeed-portal (4 usages)
      org.apache.jetspeed.aggregator.impl (1 usage)
      BasicAggregator (1 usage)
      build(RequestContext) (1 usage)
      (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext()
      org.apache.jetspeed.decoration (1 usage)
      PageTheme (1 usage)
      PageTheme(Page, DecorationFactory, RequestContext) (1 usage)
      (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
      org.apache.jetspeed.layout.impl (2 usages)
      AddPortletAction (1 usage)
      run(RequestContext, Map) (1 usage)
      >>>> (126, 18) root.getFragments().add(fragment);
      PortletPlacementContextImpl (1 usage)
      processFragment(Fragment) (1 usage)
      (145, 29) List children = fragment.getFragments();
      jetspeed-registry (1 usage)
      org.apache.jetspeed.components.portletentity (1 usage)
      TestPortletEntityDAO.ContentFragmentImpl (1 usage)
      getFragments() (1 usage)
      (145, 22) return f.getFragments();

      1. permissions-fragment2.diff
        81 kB
        David Jencks
      2. permissions-fragment2.jar
        54 kB
        David Jencks

        Activity

        David Jencks created issue -
        Hide
        David Jencks added a comment -

        On further thought, all or almost all of these problems can be solved by adding add and remove methods to Fragment that add and remove to the private fragment list, before security filtering.

        Show
        David Jencks added a comment - On further thought, all or almost all of these problems can be solved by adding add and remove methods to Fragment that add and remove to the private fragment list, before security filtering.
        Hide
        David Jencks added a comment -

        I've implemented a possible solution for this by adding methods to the Fragment interface:
        addFragment
        removeFragment
        setFragments

        However, almost all the files involved were already changed in my jetspeed copy from the work on JS2-475. I'm attaching files that are modified from my first patch to JS2-475 to this issue as an svk diff and the entire files. I will attach full diffs as well to JS2-475. If JS2-475 is applied I can try to make a diff for this issue only. Comparing the diffs for JS2-475 and this issue will show what is actually changed for this issue.

        Show
        David Jencks added a comment - I've implemented a possible solution for this by adding methods to the Fragment interface: addFragment removeFragment setFragments However, almost all the files involved were already changed in my jetspeed copy from the work on JS2-475 . I'm attaching files that are modified from my first patch to JS2-475 to this issue as an svk diff and the entire files. I will attach full diffs as well to JS2-475 . If JS2-475 is applied I can try to make a diff for this issue only. Comparing the diffs for JS2-475 and this issue will show what is actually changed for this issue.
        David Jencks made changes -
        Field Original Value New Value
        Attachment permissions-fragment2.diff [ 12322226 ]
        Attachment permissions-fragment2.jar [ 12322225 ]
        Hide
        Randy Watler added a comment -

        Yes, this is a bug as you read in the comment... constraints and permissions checks on individual fragments are relatively new. We made that change too close to the 2.0 cutoff, so we were not in the mood to change the PageManager API then... .

        I am fine with the proposed API change, but I cannot read the diffs very well. I am concerned about the DB version of the FragmentImpl since it appears you moved functionality into the constructor of this persistent class. I think we should probably go ahead and apply the patch when others get s few moments to comment. Then, I can more propery review the changes.

        Show
        Randy Watler added a comment - Yes, this is a bug as you read in the comment... constraints and permissions checks on individual fragments are relatively new. We made that change too close to the 2.0 cutoff, so we were not in the mood to change the PageManager API then... . I am fine with the proposed API change, but I cannot read the diffs very well. I am concerned about the DB version of the FragmentImpl since it appears you moved functionality into the constructor of this persistent class. I think we should probably go ahead and apply the patch when others get s few moments to comment. Then, I can more propery review the changes.
        Hide
        David Jencks added a comment -

        Latest version of these changes in JS2-444 geronimo-jetspeed10.zip

        Show
        David Jencks added a comment - Latest version of these changes in JS2-444 geronimo-jetspeed10.zip
        Hide
        Randy Watler added a comment -

        I am still pondering this fix. I think there is an easy way to fix this w/o changing the API. I'll keep you posted.

        Show
        Randy Watler added a comment - I am still pondering this fix. I think there is an easy way to fix this w/o changing the API. I'll keep you posted.
        Hide
        Randy Watler added a comment -

        This problem has been resolved with a lighter footprint fix that is compatible with the 2.0.1 release by preserving the getFragments() API, (i.e. by supportting a mutable collection in all cases).

        The PageManager API has been ripe for a major refactoring for some time. Elimination of mutable collections should be considered then as an API design goal.

        Show
        Randy Watler added a comment - This problem has been resolved with a lighter footprint fix that is compatible with the 2.0.1 release by preserving the getFragments() API, (i.e. by supportting a mutable collection in all cases). The PageManager API has been ripe for a major refactoring for some time. Elimination of mutable collections should be considered then as an API design goal.
        Randy Watler made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.1-dev [ 12310686 ]
        Assignee Randy Watler [ rwatler ]
        Hide
        Ate Douma added a comment -

        Closed again now properly recorded against Fix Version 2.1 as well

        Show
        Ate Douma added a comment - Closed again now properly recorded against Fix Version 2.1 as well
        Ate Douma made changes -
        Fix Version/s 2.1 [ 12310617 ]
        Ate Douma made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        4d 12h 59m 1 Randy Watler 24/Jan/06 16:15
        Resolved Resolved Closed Closed
        2079d 5h 23m 1 Ate Douma 04/Oct/11 22:38

          People

          • Assignee:
            Randy Watler
            Reporter:
            David Jencks
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development