Skip to content

Commit

Permalink
bridge: Fix fsreplace1 crash for error cases
Browse files Browse the repository at this point in the history
Catch errors from `rename()` and translate them into an `access-denied`
(e.g. the specified `path` is a directory) or `not-found` (directory is
deleted underneath us) channel error instead of crashing the bridge.

Likewise, intercept `FileNotFoundError` when trying to create a temp
file in a nonexisiting directory. That previously returned
`internal-error`, but it's really not. We don't have a really good
exisiting problem code, but `access-denied` is closest.
  • Loading branch information
martinpitt committed Feb 13, 2024
1 parent 643dc12 commit 5df5458
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 3 deletions.
17 changes: 14 additions & 3 deletions src/cockpit/channels/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ def do_data(self, data):
continue
except PermissionError as exc:
raise ChannelError('access-denied') from exc
except FileNotFoundError as exc:
# directory of path does not exist
raise ChannelError('not-found') from exc
except OSError as exc:
raise ChannelError('internal-error', message=str(exc)) from exc
else:
Expand Down Expand Up @@ -212,10 +215,18 @@ def do_done(self):

try:
os.rename(self._temppath, self._path)
except OSError:
# ensure to not leave the temp file behind
# ensure to not leave the temp file behind
except FileNotFoundError as exc:
self.unlink_temppath()
raise
raise ChannelError('not-found', message=str(exc)) from exc
except IsADirectoryError as exc:
self.unlink_temppath()
# not ideal, but the closest code we have
raise ChannelError('access-denied', message=str(exc)) from exc
except OSError as exc:
self.unlink_temppath()
raise ChannelError('internal-error', message=str(exc)) from exc

self._tempfile.close()
self._tempfile = None

Expand Down
15 changes: 15 additions & 0 deletions test/pytest/test_bridge.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,21 @@ async def test_fsreplace1(transport: MockTransport, tmp_path: Path) -> None:
assert not myfile.exists()


@pytest.mark.asyncio
async def test_fsreplace1_error(transport: MockTransport, tmp_path: Path) -> None:
# trying to write a directory
ch = await transport.check_open('fsreplace1', path=str(tmp_path))
transport.send_data(ch, b'not me')
transport.send_done(ch)
await transport.assert_msg('', command='close', channel=ch, problem='access-denied')

# nonexisting directory
ch = await transport.check_open('fsreplace1', path='/non/existing/file')
transport.send_data(ch, b'not me')
transport.send_done(ch)
await transport.assert_msg('', command='close', channel=ch, problem='not-found')


@pytest.mark.asyncio
@pytest.mark.parametrize('channeltype', CHANNEL_TYPES)
async def test_channel(bridge: Bridge, transport: MockTransport, channeltype, tmp_path: Path) -> None:
Expand Down

0 comments on commit 5df5458

Please sign in to comment.