Cool, thanks!
On Thu, Jun 20, 2019 at 5:25 PM Nir Soffer <nirsof(a)gmail.com> wrote:
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
>>
>>