bugfix/1247: Don't create an lmdb library if the name is invalid. #1481

muhammadhamzasajjad · 2024-04-08T18:39:50Z

Reference Issues/PRs

Fixes #1247

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp

cpp/arcticdb/util/name_validation.cpp

DrNickClarke · 2024-04-16T13:07:19Z

Looks good to me

vasil-pashov · 2024-04-16T13:28:59Z

python/tests/integration/arcticdb/test_lmdb.py

+def test_invalid_lib_name_windows(lmdb_storage):
+    ac = lmdb_storage.create_arctic()
+
+    invalid_paths = ["lib?1", "lib:1", "lib|1", "lib.", "lib "]
+    for lib in invalid_paths:
+        with pytest.raises(UserInputException) as e:
+            ac.create_library(lib)
+        assert ac.list_libraries() == []
+
+    valid_libs = ["lib#~@,1", "lib{)[.1", "!lib$%^"]
+    for lib in valid_libs:
+        ac.create_library(lib)
+    assert set(ac.list_libraries()) == set(valid_libs)


I'd split this into two separate test cases because it looks like it's testing two separate things. One is that some strings are valid and the other is that some strings are invalid. The name is confusing as well. Also it looks like the naming is inconsistent, it's paths in one place and libs in the other. I'd also use parametrized test in order to avoid the for lib in invalid_paths loop. It would look a lot cleaner as the test would have only the asserts in it's body.

vasil-pashov · 2024-04-16T13:29:59Z

python/tests/integration/arcticdb/test_lmdb.py

+@pytest.mark.skipif(sys.platform == "win32", reason="Windows has different file path name restrictions")
+def test_valid_lib_name_linux(lmdb_storage):
+    ac = lmdb_storage.create_arctic()
+    valid_libs = ["lib?1", "lib:1", "lib|1", "lib ", "lib#~@,1", "lib{)[.1", "!lib$%^"]


Use parametrized test

vasil-pashov · 2024-04-16T16:00:20Z

python/tests/integration/arcticdb/test_lmdb.py

+def is_valid_lmdb_lib_name(lmdb_storage, lib_name):
+    ac = lmdb_storage.create_arctic()
+    ac.create_library(lib_name)
+
+    return ac.list_libraries() == [lib_name]


I'd add this inside the tests instead of separating it in a function. While it helps avoiding code duplication it has the side effect of actually creating a library. I wouldn't expect a function checking if the name is valid to create a library.

alexowens90 · 2024-04-17T10:15:40Z

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp

@@ -259,6 +259,23 @@ void LmdbStorage::do_iterate_type(KeyType key_type, const IterateTypeVisitor& vi
    }
 }

+bool LmdbStorage::do_is_path_valid(const std::string_view pathString) const {
+#ifdef _WIN32
+    std::string_view invalid_win32_chars = "<>:\"|?*";


This list is not exhaustive:
https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names
There are also a whole bunch of other reserved filenames that would fail
https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file

Note that when we create an lmdb library, it creates a folder with the given name, therefore I allow / and \ as in this case if you give name1/name2 it will just create a subdir name2 under name1 and everything else (e.g. reads and writes on the lib) will work fine. In fact, this older test expects to be able to create such libraries. The purpose of this fix is to strictly disallow library names that are disallowed by windows, and a library name containing / or \ would not fail so they should be allowed.

I did look at the list of other reserved filenames while working on the fix. But the docs were strictly saying "reserved names for the name of a file". In fact, I tested this and I was able to create libraries with names such as CON, PRN, COM0 which successfully created folders with these names. These are reserved names but not strictly disallowed and that's why I allowed these names.

Aha OK that makes sense. I think it's worth adding a comment here explaining all this, as it won't be obvious to the next person reading it.

alexowens90 · 2024-04-17T10:16:03Z

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp

+        return false;
+    }
+#else
+    (void) pathString; // suppress -Werror=unused-parameter


Use ARCTICDB_UNUSED instead of this

alexowens90 · 2024-04-17T10:17:42Z

cpp/arcticdb/util/name_validation.cpp

    verify_name("library name", library_path, true, UNSUPPORTED_S3_CHARS);
+    if(!store->is_path_valid(library_path)) {


You can get rid of this if using user_input::check

python/tests/integration/arcticdb/test_lmdb.py

@alexowens90

#### Reference Issues/PRs Addresses @alexowens90's comments in #1481 after that PR was merged. #### What does this implement or fix? #### Any other comments? #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>

) #### Reference Issues/PRs Fixes #1247 #### What does this implement or fix? #### Any other comments? #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>

@alexowens90

#### Reference Issues/PRs Addresses @alexowens90's comments in #1481 after that PR was merged. #### What does this implement or fix? #### Any other comments? #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>

muhammadhamzasajjad self-assigned this Apr 8, 2024

muhammadhamzasajjad force-pushed the lmdb_invalid_lib_name branch from 9b0669d to 9ae6d49 Compare April 8, 2024 18:45

muhammadhamzasajjad added the bug Something isn't working label Apr 9, 2024

vasil-pashov reviewed Apr 9, 2024

View reviewed changes

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp Outdated Show resolved Hide resolved

vasil-pashov reviewed Apr 9, 2024

View reviewed changes

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp Outdated Show resolved Hide resolved

vasil-pashov reviewed Apr 9, 2024

View reviewed changes

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp Outdated Show resolved Hide resolved

vasil-pashov reviewed Apr 9, 2024

View reviewed changes

cpp/arcticdb/util/name_validation.cpp Outdated Show resolved Hide resolved

muhammadhamzasajjad force-pushed the lmdb_invalid_lib_name branch 3 times, most recently from 8b5fc3f to 60a1161 Compare April 15, 2024 16:40

muhammadhamzasajjad requested a review from vasil-pashov April 16, 2024 12:56

muhammadhamzasajjad marked this pull request as ready for review April 16, 2024 12:57

vasil-pashov reviewed Apr 16, 2024

View reviewed changes

muhammadhamzasajjad added 2 commits April 16, 2024 16:32

Check invalid lmdb lib names.

12c62cb

Refactor lmdb valid/invalid lib names tests

1504e18

muhammadhamzasajjad force-pushed the lmdb_invalid_lib_name branch from bf863a3 to 1504e18 Compare April 16, 2024 15:42

vasil-pashov reviewed Apr 16, 2024

View reviewed changes

remove is_valid_lib_name helper

89d7808

muhammadhamzasajjad requested a review from vasil-pashov April 16, 2024 22:18

vasil-pashov approved these changes Apr 17, 2024

View reviewed changes

muhammadhamzasajjad merged commit 1572029 into master Apr 17, 2024
115 checks passed

muhammadhamzasajjad deleted the lmdb_invalid_lib_name branch April 17, 2024 09:17

alexowens90 reviewed Apr 17, 2024

View reviewed changes

muhammadhamzasajjad added a commit that referenced this pull request Apr 17, 2024

bugfix/1247:address comments in #1481

4606cb8

muhammadhamzasajjad mentioned this pull request Apr 17, 2024

bugfix/1247:address comments in #1481 #1503

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix/1247: Don't create an lmdb library if the name is invalid. #1481

bugfix/1247: Don't create an lmdb library if the name is invalid. #1481

muhammadhamzasajjad commented Apr 8, 2024

DrNickClarke commented Apr 16, 2024

vasil-pashov Apr 16, 2024 •

edited

Loading

vasil-pashov Apr 16, 2024

vasil-pashov Apr 16, 2024

alexowens90 Apr 17, 2024

muhammadhamzasajjad Apr 17, 2024 •

edited

Loading

muhammadhamzasajjad Apr 17, 2024

alexowens90 Apr 17, 2024

alexowens90 Apr 17, 2024

alexowens90 Apr 17, 2024

		verify_name("library name", library_path, true, UNSUPPORTED_S3_CHARS);
		if(!store->is_path_valid(library_path)) {

bugfix/1247: Don't create an lmdb library if the name is invalid. #1481

bugfix/1247: Don't create an lmdb library if the name is invalid. #1481

Conversation

muhammadhamzasajjad commented Apr 8, 2024

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

DrNickClarke commented Apr 16, 2024

vasil-pashov Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

vasil-pashov Apr 16, 2024

Choose a reason for hiding this comment

vasil-pashov Apr 16, 2024

Choose a reason for hiding this comment

alexowens90 Apr 17, 2024

Choose a reason for hiding this comment

muhammadhamzasajjad Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

muhammadhamzasajjad Apr 17, 2024

Choose a reason for hiding this comment

alexowens90 Apr 17, 2024

Choose a reason for hiding this comment

alexowens90 Apr 17, 2024

Choose a reason for hiding this comment

alexowens90 Apr 17, 2024

Choose a reason for hiding this comment

vasil-pashov Apr 16, 2024 •

edited

Loading

muhammadhamzasajjad Apr 17, 2024 •

edited

Loading