allenai / ScienceWorld

ScienceWorld is a text-based virtual environment centered around accomplishing tasks from the standardized elementary science curriculum.
https://sciworld.apps.allenai.org/
Apache License 2.0
199 stars 24 forks source link

Prevent use-after-free in ScienceWorldEnv.shutdown #24

Closed aphedges closed 1 year ago

aphedges commented 2 years ago

I discovered a bug in the Python wrapper when using code based on the examples. The line random_agent.py:L136 shows the use of shutdown(), but including the call causes an exception to be thrown. I have created a minimal example script and reproduced the resulting error:

import logging
from scienceworld import ScienceWorldEnv
logging.basicConfig(level=logging.INFO)
env = ScienceWorldEnv("")
env.shutdown()
$ python gateway_bug.py 
Launching ScienceWorld Server (Port 25335) -- this may take a moment.
Load:  (variation: 0) (simplifications: )
INFO:py4j.java_gateway:Exception while shutting down callback server
Traceback (most recent call last):
  File "/Users/ahedges/.pyenv/versions/sciworld/lib/python3.8/site-packages/py4j/java_gateway.py", line 1983, in shutdown
    self._gateway_client.shutdown_gateway()
  File "/Users/ahedges/.pyenv/versions/sciworld/lib/python3.8/site-packages/py4j/java_gateway.py", line 1006, in shutdown_gateway
    connection = self._get_connection()
  File "/Users/ahedges/.pyenv/versions/sciworld/lib/python3.8/site-packages/py4j/java_gateway.py", line 980, in _get_connection
    raise Py4JNetworkError("Gateway is not connected.")
py4j.protocol.Py4JNetworkError: Gateway is not connected.

In commit 650a0f741679ab007e3e6619ebd23e01dd3c8a2c, a check was added to only call self.gateway.shutdown() if self.gateway existed. self.gateway.shutdown() throws an exception if called while shut down, so this seems like a reasonable precaution. However, self.gateway almost always exists in ScienceWorldEnv. It is declared in ScienceWorldEnv.__init__, and no code deletes it. The manual call to ScienceWorldEnv.shutdown properly shuts down the gateway, but when the script ends and CPython's GC calls ScienceWorldEnv.__del__ (see object.__del__ for more information), self.gateway.shutdown() is called a second time.

The solution to the problem is simple: delete self.gateway after it has been shut down so the existing hasattr works as was presumably intended.

MarcCote commented 1 year ago

You are right. Thank you for the fix.