New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent use of the same array in CollectionProperty (close #58743) #293

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
4 participants
@emilianbold

emilianbold commented Apr 26, 2017

CollectionProperty constructor receives an empty ObjectTableModel.objects
collection but normalizeList will return it as-is instead of creating a
new empty collection.

The two classes share the same array but add different kind of classes.

NOTE: Basically the bug is in AbstractProperty.normalizeList because it should never reuse the same instance. I've done the patch as you see it because it has the lowest impact and does not change the normalizeList API (perhaps reusing is expected in some cases?)

Let me know if I should redo the patch by just changing normalizeList!

Constructor stacktrace:
at org.apache.jmeter.testelement.property.CollectionProperty.(CollectionProperty.java:40)
at org.apache.jmeter.testelement.property.AbstractProperty.makeProperty(AbstractProperty.java:395)
at org.apache.jmeter.testelement.property.AbstractProperty.createProperty(AbstractProperty.java:368)
at org.apache.jmeter.testbeans.gui.TestBeanGUI.setPropertyInElement(TestBeanGUI.java:277)
at org.apache.jmeter.testbeans.gui.TestBeanGUI.modifyTestElement(TestBeanGUI.java:266)
at org.apache.jmeter.gui.tree.JMeterTreeModel.addComponent(JMeterTreeModel.java:148)
at org.apache.jmeter.gui.action.AddToTree.doAction(AddToTree.java:70)
at org.apache.jmeter.gui.action.ActionRouter.performAction(ActionRouter.java:74)
at org.apache.jmeter.gui.action.ActionRouter.lambda$actionPerformed$28(ActionRouter.java:59)
at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311)

@FSchumacher

This comment has been minimized.

Show comment
Hide comment
@FSchumacher

FSchumacher Apr 26, 2017

Contributor

Great. Thanks for your analysis and your fix.

I tend to change the behaviour of normalizeList, even if it is a change in the unwritten contract. The current behaviour seems wrong to me. It was probably meant to save space.

Contributor

FSchumacher commented Apr 26, 2017

Great. Thanks for your analysis and your fix.

I tend to change the behaviour of normalizeList, even if it is a change in the unwritten contract. The current behaviour seems wrong to me. It was probably meant to save space.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Apr 26, 2017

Contributor

Thanks for analysis and patch. It was a hard one !.

@FSchumacher , I don't understand your comment , what exactly do you want to change ?
Thanks

Contributor

pmouawad commented Apr 26, 2017

Thanks for analysis and patch. It was a hard one !.

@FSchumacher , I don't understand your comment , what exactly do you want to change ?
Thanks

@FSchumacher

This comment has been minimized.

Show comment
Hide comment
@FSchumacher

FSchumacher Apr 26, 2017

Contributor

As Emilan suggested, we could change normalizeList to always create a new collection, instead of introducing a new method, that has configurable behaviour.

I consider the current behaviour to be a bug.

Contributor

FSchumacher commented Apr 26, 2017

As Emilan suggested, we could change normalizeList to always create a new collection, instead of introducing a new method, that has configurable behaviour.

I consider the current behaviour to be a bug.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Apr 26, 2017

Contributor

@FSchumacher , I agree. Let's commit the patch without the new method.

Contributor

pmouawad commented Apr 26, 2017

@FSchumacher , I agree. Let's commit the patch without the new method.

@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold Apr 26, 2017

Ammeding the commit as we speak.

emilianbold commented Apr 26, 2017

Ammeding the commit as we speak.

@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold Apr 26, 2017

Done. The new patch is much cleaner.

emilianbold commented Apr 26, 2017

Done. The new patch is much cleaner.

@FSchumacher

This comment has been minimized.

Show comment
Hide comment
@FSchumacher

FSchumacher Apr 26, 2017

Contributor

Great, I wonder if we should change the return value from null to Collections.emptyList() in case of an Exception, too.

I will add a test case to the bugzilla entry for normalizeList.

Contributor

FSchumacher commented Apr 26, 2017

Great, I wonder if we should change the return value from null to Collections.emptyList() in case of an Exception, too.

I will add a test case to the bugzilla entry for normalizeList.

@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold Apr 26, 2017

It's unclear to me why it's so important to use the same kind of collection class. So I tried to keep the patch low-impact.

Instead of null or an empty list it could return a new ArrayList with the objects.

emilianbold commented Apr 26, 2017

It's unclear to me why it's so important to use the same kind of collection class. So I tried to keep the patch low-impact.

Instead of null or an empty list it could return a new ArrayList with the objects.

@vherilier

This comment has been minimized.

Show comment
Hide comment
@vherilier

vherilier Apr 27, 2017

Hi guys,

could be the same issue available with the next method "normalizeMap"?

vherilier commented Apr 27, 2017

Hi guys,

could be the same issue available with the next method "normalizeMap"?

@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold Apr 27, 2017

could be the same issue available with the next method "normalizeMap"?

Actually, yes! Should I update this patch or leave normalizeMap for another fix?

emilianbold commented Apr 27, 2017

could be the same issue available with the next method "normalizeMap"?

Actually, yes! Should I update this patch or leave normalizeMap for another fix?

@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold Apr 27, 2017

Like I've said, I wanted my patch to be low-impact. Ideally normalizeList should be something like this:

    protected Collection<JMeterProperty> normalizeList(Collection<?> coll) {
        return coll.stream()
                .map(this::convertObject)
                .collect(Collectors.toCollection(ArrayList::new));
    }

Then normalizeMap should be like this:

    protected Map<String, JMeterProperty> normalizeMap(Map<?,?> coll) {
        return coll.entrySet()
                .stream()
                .collect(Collectors.toMap(entry -> {
                    Object key = entry.getKey();

                    if (key == null) {
                        return null;
                    } else {
                        if (!(key instanceof String)) {
                            log.error("Expected key type String, found: {}", key.getClass());
                        }
                        return key.toString();
                    }
                }, entry -> convertObject(entry.getValue())));
    }

emilianbold commented Apr 27, 2017

Like I've said, I wanted my patch to be low-impact. Ideally normalizeList should be something like this:

    protected Collection<JMeterProperty> normalizeList(Collection<?> coll) {
        return coll.stream()
                .map(this::convertObject)
                .collect(Collectors.toCollection(ArrayList::new));
    }

Then normalizeMap should be like this:

    protected Map<String, JMeterProperty> normalizeMap(Map<?,?> coll) {
        return coll.entrySet()
                .stream()
                .collect(Collectors.toMap(entry -> {
                    Object key = entry.getKey();

                    if (key == null) {
                        return null;
                    } else {
                        if (!(key instanceof String)) {
                            log.error("Expected key type String, found: {}", key.getClass());
                        }
                        return key.toString();
                    }
                }, entry -> convertObject(entry.getValue())));
    }
@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Apr 28, 2017

Contributor

+1 for fixing both methods

Contributor

pmouawad commented Apr 28, 2017

+1 for fixing both methods

Prevent collection reuse (close #58743)
CollectionProperty constructor receives an empty ObjectTableModel.objects
collection but normalizeList will return it as-is instead of creating a
new empty collection.

The two classes share the same collection but add different kind of classes.
@emilianbold

This comment has been minimized.

Show comment
Hide comment
@emilianbold

emilianbold commented Apr 28, 2017

Done.

@asfgit asfgit closed this in f2e65bc Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment