On Thu, Jun 20, 2019 at 4:37 PM Amit Bawer <abawer(a)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(a)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(a)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,
> §or)) {
> 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)
>
>
> -(a)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)
>
>
> -(a)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)
>
>
> -(a)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)
>
>
> -(a)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)
>
>
> -(a)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)
>
>
> -(a)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())
>
>
> -(a)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
>
>