Cool, thanks! 

On Thu, Jun 20, 2019 at 5:25 PM Nir Soffer <nirsof@gmail.com> wrote:
On Thu, Jun 20, 2019 at 4:37 PM Amit Bawer <abawer@redhat.com> wrote:
We have same limitation for lockspace paths as well, maybe we need to add this validation as part of the path converter to cover all path usages.

Sanlock have different length limitations for paths, so I think this should be solved in the API level.

In the next version I will handle also lockspace paths.

 

On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof@gmail.com> wrote:
Previously if a path was too long, we truncated the path silently. This
probably fail when sanlock try to access non-existing path with "No such
path or directory" error. If you are unlucky, this could try to access
the wrong path, failing with bogus error, returning wrong data, or
worse, writing a resource into the wrong location, destroying existing
data.

Now we fail immediately with:

    ValueError: Path is too long: '/too/long/path/...'

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 python/sanlock.c     | 17 +++++++++++++++++
 tests/python_test.py |  7 -------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index ca3713d..7c50aba 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -143,10 +143,21 @@ pystring_as_cstring(PyObject *obj)
 #else
     return PyUnicode_AsUTF8(obj);
 #endif
 }

+static int
+validate_path(PyObject *path)
+{
+    if (PyBytes_Size(path) > SANLK_PATH_LEN - 1) {
+        set_error(PyExc_ValueError, "Path is too long: %s", path);
+        return 0;
+    }
+
+    return 1;
+}
+
 static int
 parse_single_disk(PyObject* disk, struct sanlk_disk* res_disk)
 {
     int rv = 0;
     PyObject *path = NULL;
@@ -161,10 +172,13 @@ parse_single_disk(PyObject* disk, struct sanlk_disk* res_disk)
         /* Override the error since it confusing in this context. */
         set_error(PyExc_ValueError, "Invalid disk %s", disk);
         goto finally;
     }

+    if (!validate_path(path))
+        goto finally;
+
     strncpy(res_disk->path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);
     res_disk->offset = offset;
     rv = 1;

 finally:
@@ -654,10 +668,13 @@ py_read_resource(PyObject *self __unused, PyObject *args, PyObject *keywds)
     if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&|kli", kwlist,
         pypath_converter, &path, &(res->disks[0].offset), &align, &sector)) {
         goto finally;
     }

+    if (!validate_path(path))
+        goto finally;
+
     /* prepare the resource disk path */
     strncpy(res->disks[0].path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);

     /* set alignment/sector flags */
     if (add_align_flag(align, &res->flags) == -1)
diff --git a/tests/python_test.py b/tests/python_test.py
index 5624f60..47788e4 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -550,11 +550,10 @@ def test_write_resource_parse_args(no_sanlock_daemon, name, filename, encoding):

     with raises_sanlock_errno():
         sanlock.write_resource(b"ls_name", name, disks)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_write_resource_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.write_resource(b"ls_name", b"res_name", [(path, 0)])

@@ -573,11 +572,10 @@ def test_release_resource_parse_args(no_sanlock_daemon, name, filename, encoding

     with raises_sanlock_errno():
         sanlock.release(b"ls_name", name, disks)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_release_resource_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.release(b"ls_name", b"res_name", [(path, 0)])

@@ -596,11 +594,10 @@ def test_read_resource_owners_parse_args(no_sanlock_daemon, name, filename, enco

     with raises_sanlock_errno():
         sanlock.read_resource_owners(b"ls_name", name, disks)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_read_resource_owners_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.read_resource_owners(b"ls_name", b"res_name", [(path, 0)])

@@ -658,11 +655,10 @@ def test_init_resource_parse_args(no_sanlock_daemon, name, filename, encoding):
         sanlock.init_resource(b"ls_name", name, disks)
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.init_resource(name, b"res_name", disks)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_init_resource_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.init_resource(b"ls_name", b"res_name", [(path, 0)])

@@ -689,11 +685,10 @@ def test_read_resource_parse_args(no_sanlock_daemon, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
         sanlock.read_resource(path)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_read_resource_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.read_resource(path)

@@ -713,11 +708,10 @@ def test_request_parse_args(no_sanlock_daemon, name, filename, encoding):

     with raises_sanlock_errno():
         sanlock.request(name, b"res_name", disks)


-@pytest.mark.xfail(reason="path truncated silently")
 def test_request_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.request(b"ls_name", b"res_name", [(path, 0)])

@@ -737,11 +731,10 @@ def test_acquire_parse_args(no_sanlock_daemon, name, filename, encoding):

     with raises_sanlock_errno():
         sanlock.acquire(name, b"res_name", disks, pid=os.getpid())


-@pytest.mark.xfail(reason="path truncated silently")
 def test_acquire_path_length(no_sanlock_daemon):
     with pytest.raises(ValueError):
         path = "x" * constants.SANLK_PATH_LEN
         sanlock.acquire(b"ls_name", b"res_name", [(path, 0)], pid=os.getpid())

--
2.17.2