If the value in map is the map's self, the new new JSONObject(map) cause StackOverflowError

[BIngDiAn-s]

poc as follw:

Map<Object,Object> a=new HashMap<>(); a.put(" ",a); JSONObject A=new JSONObject(a);

[eedijs]

Could maybe add another if in there to check if the entry (in the for loop) equals the argument itself, and throw an exception?

    public JSONObject(Map<?, ?> m) {
        if (m == null) {
            this.map = new HashMap<String, Object>();
        } else {
            this.map = new HashMap<String, Object>(m.size());
        	for (final Entry<?, ?> e : m.entrySet()) {
        	    if(e.getKey() == null) {
        	        throw new NullPointerException("Null key.");
        	    }
                final Object value = e.getValue();
                if (m.equals(value)) {
                    throw new Exception("Message");
                }
                if (value != null) {
                    this.map.put(String.valueOf(e.getKey()), wrap(value));
                }
            }
        }
    }

[stleary]

@eedijs Can you provide an example of a JSON doc that causes a StackOverflowError?

[eedijs]

@eedijs Can you provide an example of a JSON doc that causes a StackOverflowError?

I don't have an actual example of a JSON where this would happen (maybe the author of this issue does), I'm not even sure in what case you would put a map into itself, but this test asserts that it throws the exception:

    @Test
    public void jsonObjectFromMap() {
        // given
        Map<String, Object> map = new HashMap<>();
        map.put("itself", map);

        // when - then
        assertThrows(StackOverflowError.class, () -> new JSONObject(map));
    }

[stleary]

Sorry, never mind, @BIngDiAn-s included a proof-of-concept when opening this issue, it's in the first post. Your approach might work if the duplicate map is on the top level as in the example, but not sure what might happen if the duplicate was more deeply nested. Perhaps the method should be wrapped in a try/catch block?

[ZachsCoffee]

@stleary If we want to check for circle dependency (this is the case for this issue) we have to keep a set of the instances that are wrap() and when we see the same instance a second time throw a circle dependency exception. I can provide an implementation. Edit: As I saw now there already exists this functionality org.json.JSONObject#JSONObject(java.lang.Object, java.util.Set<java.lang.Object>) but for another case.

Can I proceed with the PR?

[stleary]

@ZachsCoffee I don't think this is a high-priority issue. There are numerous comments in the code that warn against using cyclical data structures, which until now has been sufficient. Adding more features burdens the code with additional complexity and does not serve the core purpose, which is to be a reference app. A pull request that checks for cycles has a low probability of success.

However, if you want to go ahead, it must be strictly opt-in, due to the expected performance cost. I don't believe there is currently a mechanism for this. There is a ParserConfiguration class, but I believe it is only used for XML and JSONML parsing. I suppose you might be able to extend its use for regular JSON parsing, but not sure if that is possible, or how it would work.

If anyone else has opinions on this issue, please post them here.

[ZachsCoffee]

@stleary I open the PR. The same functionality already exists in the same class to not allow circular dependency but for another case. If you think the track for circular dependency isn't necessary for any reason I don't have any problem with my PR being closed (it was 5m work anyway) :)

[ZachsCoffee]

Sorry @stleary, I read again your comment and now I realize what you mean about the opt-in for the circular dependency tracking. I agree that the ParserConfiguration isn't a good fit for the job and in general it is hard to have a common configuration that fits well for different things. So I think it will be better to create a new configuration for the JSON format. Do you want to proceed with the creation of a new configuration class for the JSON?

[stleary]

@ZachsCoffee No worries, Sure, you can proceed with adding a new config class. Recommend using ParserConfiguration and its usages as a model or starting point.

[rudrajyotib]

Hello @stleary @ZachsCoffee - In my opinion, the cyclic reference check should be a mandate and not a configuration. We may configure, what is an acceptable depth, with a max value (may be 1000), but every time the Map based constructor is invoked, the Map itself should be filtered before accepting.

[rudrajyotib]

@stleary @ZachsCoffee - Is this issue being worked upon ?

[stleary]

Closed due to issue has been fixed in another commit.