Skip to content

Fix MemoryFileSystem._internal_path prefix stripping corruption - #429

Draft
rootkiller6788 wants to merge 1 commit into
google:mainfrom
rootkiller6788:fix-memory-fs-internal-path
Draft

Fix MemoryFileSystem._internal_path prefix stripping corruption#429
rootkiller6788 wants to merge 1 commit into
google:mainfrom
rootkiller6788:fix-memory-fs-internal-path

Conversation

@rootkiller6788

Copy link
Copy Markdown

Problem

MemoryFileSystem._internal_path stripped the /mem/ prefix with:

return '/' + resolve_path(path).lstrip(self._prefix)

str.lstrip treats its argument as a set of characters, not a literal prefix. So any leading m or e characters after the prefix were also stripped, corrupting path components:

Input Old internal path
/mem/models/a /odels/a
/mem/eval/b /val/b
/mem/memory /ory

This is observable via listdir (fs.mkdirs('/mem/models/a') then fs.listdir('/mem') returns ['odels'] instead of ['models']) and causes distinct paths such as /mem/models and /mem/odels to collide.

Fix

Replace lstrip with an explicit startswith check plus a slice, and keep mapping the bare prefix /mem to the root:

def _internal_path(self, path):
  path = resolve_path(path)
  if path.startswith(self._prefix):
    path = path[len(self._prefix):]
  elif path == self._prefix.rstrip('/'):
    path = ''
  return '/' + path

Tests

Added MemoryFileSystemTest.test_internal_path_prefix_stripping, which verifies that m/e-prefixed components are preserved, and that /mem/models and /mem/odels remain distinct.

str.lstrip treats its argument as a set of characters, so
resolve_path(path).lstrip('/mem/') also strips any leading 'm'/'e'
characters after the prefix. This corrupts path components that begin
with those characters: /mem/models becomes 'odels' and /mem/eval becomes
'val', and /mem/models collides with the distinct path /mem/odels.

Replace it with an explicit prefix check plus slice, and keep mapping
the bare prefix ('/mem') to the root. Add a regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant