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.jar
        54 kB
        David Jencks
      2. permissions-fragment2.diff
        81 kB
        David Jencks

        Activity

        No work has yet been logged on this issue.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development