Closed bignose-debian closed 1 year ago
An easy way to have the get_config
call fail, is to have a badly-formatted configuration file that get_config
will read (such as pyproject.toml
). If get_config
raises a ValueError
it will be erroneously caught in the get_context_from_config
logic as described above.
I have implemented a test case which demonstrates this, and a simple change to correct it. See the branch refine-exception-handling in my personal fork.
The test case (after refactoring to allow adding this test case) is:
def test_noenv_nodef_invalid_setup(self):
"""
Setup: $IPDB_CONFIG unset, $HOME/.ipdb does not exist,
setup.cfg does not exist, pyproject.toml content is invalid.
Result: Propagate exception from `get_config`.
"""
os.unlink(self.env_filename)
os.unlink(self.default_filename)
os.unlink(self.setup_filename)
write_lines_to_file(
self.pyproject_filename,
[
"[ipdb]",
"spam = abc",
],
)
with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd):
try:
__ = get_context_from_config()
except TomlDecodeError:
pass
else:
self.fail("Expected TomlDecodeError from invalid config file")
The correction is as simple as moving a line out of the exception handling so that its exceptions propagate normally:
modified ipdb/__main__.py
@@ -80,8 +80,8 @@ def set_trace(frame=None, context=None, cond=True):
def get_context_from_config():
+ parser = get_config()
try:
- parser = get_config()
return parser.getint("ipdb", "context")
except (configparser.NoSectionError, configparser.NoOptionError):
return 3
Thanks for the analysis and the test !
Any reason this is not a PR ?
On 09-Sep-2021, Godefroid Chapelle wrote:
Thanks for the analysis and the test !
You're welcome, I hope it helps.
Any reason this is not a PR ?
While Git is entirely distributed and allows us to collaborate, the GitHub proprietary PR system doesn't work for repositories hosted outside GitHub.
-- \ “The Things to do are: the things that need doing, that you see | `\ need to be done, and that no one else seems to see need to be | _o__) done.” —Richard Buckminster Fuller, 1970-02-16 | Ben Finney @.***>
@bignose-debian I am fine adding your repo as remote to merge your changes myself. Can you point me to the proper https git url ?
@bignose-debian I am fine adding your repo as remote to merge your changes myself. Can you point me to the proper https git url ?
Yes, see the summary page for that repository.
Your work has been included in master. See 333605e54db1eb224019eb378bb34bb72dc5d950.
Thanks a lot !
Released in https://pypi.org/project/ipdb/0.13.11/
The exception handling in
get_context_from_config
makes incorrect assumptions, and catches exceptions that it does not handle correctly.In
get_context_from_config
, the statementsare wrapped in a
try
block. But theexcept
clauses evidently anticipate exceptions from the second statement only: they expect that theparser
object exists and it was theparser.getint
call that raised an exception.This assumption is incorrect.
When the call to
get_config
raises aValueError
, it is erroneously caught as though raised from an already existingparser
. Then theexcept
clause attempts to use that object and it fails during exception handling.