[prev in list] [next in list] [prev in thread] [next in thread]
List: sanlock-devel
Subject: Re: [PATCH v2 8/8] python: Fail with ValueError if disk path is too long
From: Amit Bawer <abawer () redhat ! com>
Date: 2019-06-20 14:58:02
Message-ID: CAOfZdw_z9ngUgdxAF_pP80ZU6y+2ot3+CZGWQAVLb-VJUWLnPg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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,
>>> §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)
>>>
>>>
>>> -@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
>>>
>>>
[Attachment #5 (text/html)]
<div dir="ltr">Cool, thanks! <br></div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Thu, Jun 20, 2019 at 5:25 PM Nir Soffer <<a \
href="mailto:nirsof@gmail.com">nirsof@gmail.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div \
class="gmail_default" style="font-size:large"><span style="font-size:small">On Thu, \
Jun 20, 2019 at 4:37 PM Amit Bawer <<a href="mailto:abawer@redhat.com" \
target="_blank">abawer@redhat.com</a>> wrote:</span><br></div></div><div \
class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.</div></blockquote><div><br></div><div><div class="gmail_default" \
style="font-size:large">Sanlock have different length limitations for paths, so I \
think this should be solved in the API level.</div><div class="gmail_default" \
style="font-size:large"><br></div><div class="gmail_default" \
style="font-size:large">In the next version I will handle also lockspace \
paths.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px \
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jun 16, 2019 at 11:50 \
PM Nir Soffer <<a href="mailto:nirsof@gmail.com" \
target="_blank">nirsof@gmail.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">Previously if a path was too long, we truncated \
the path silently. This<br> probably fail when sanlock try to access non-existing \
path with "No such<br> path or directory" error. If you are unlucky, this \
could try to access<br> the wrong path, failing with bogus error, returning wrong \
data, or<br> worse, writing a resource into the wrong location, destroying \
existing<br> data.<br>
<br>
Now we fail immediately with:<br>
<br>
ValueError: Path is too long: '/too/long/path/...'<br>
<br>
Signed-off-by: Nir Soffer <<a href="mailto:nsoffer@redhat.com" \
target="_blank">nsoffer@redhat.com</a>><br>
---<br>
python/sanlock.c | 17 +++++++++++++++++<br>
tests/python_test.py | 7 -------<br>
2 files changed, 17 insertions(+), 7 deletions(-)<br>
<br>
diff --git a/python/sanlock.c b/python/sanlock.c<br>
index ca3713d..7c50aba 100644<br>
--- a/python/sanlock.c<br>
+++ b/python/sanlock.c<br>
@@ -143,10 +143,21 @@ pystring_as_cstring(PyObject *obj)<br>
#else<br>
return PyUnicode_AsUTF8(obj);<br>
#endif<br>
}<br>
<br>
+static int<br>
+validate_path(PyObject *path)<br>
+{<br>
+ if (PyBytes_Size(path) > SANLK_PATH_LEN - 1) {<br>
+ set_error(PyExc_ValueError, "Path is too long: %s", path);<br>
+ return 0;<br>
+ }<br>
+<br>
+ return 1;<br>
+}<br>
+<br>
static int<br>
parse_single_disk(PyObject* disk, struct sanlk_disk* res_disk)<br>
{<br>
int rv = 0;<br>
PyObject *path = NULL;<br>
@@ -161,10 +172,13 @@ parse_single_disk(PyObject* disk, struct sanlk_disk* \
res_disk)<br>
/* Override the error since it confusing in this context. */<br>
set_error(PyExc_ValueError, "Invalid disk %s", disk);<br>
goto finally;<br>
}<br>
<br>
+ if (!validate_path(path))<br>
+ goto finally;<br>
+<br>
strncpy(res_disk->path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);<br>
res_disk->offset = offset;<br>
rv = 1;<br>
<br>
finally:<br>
@@ -654,10 +668,13 @@ py_read_resource(PyObject *self __unused, PyObject *args, \
PyObject *keywds)<br>
if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&|kli", \
kwlist,<br>
pypath_converter, &path, &(res->disks[0].offset), \
&align, &sector)) {<br> goto finally;<br>
}<br>
<br>
+ if (!validate_path(path))<br>
+ goto finally;<br>
+<br>
/* prepare the resource disk path */<br>
strncpy(res->disks[0].path, PyBytes_AsString(path), SANLK_PATH_LEN - \
1);<br> <br>
/* set alignment/sector flags */<br>
if (add_align_flag(align, &res->flags) == -1)<br>
diff --git a/tests/python_test.py b/tests/python_test.py<br>
index 5624f60..47788e4 100644<br>
--- a/tests/python_test.py<br>
+++ b/tests/python_test.py<br>
@@ -550,11 +550,10 @@ def test_write_resource_parse_args(no_sanlock_daemon, name, \
filename, encoding):<br> <br>
with raises_sanlock_errno():<br>
sanlock.write_resource(b"ls_name", name, disks)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_write_resource_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.write_resource(b"ls_name", b"res_name", \
[(path, 0)])<br> <br>
@@ -573,11 +572,10 @@ def test_release_resource_parse_args(no_sanlock_daemon, name, \
filename, encoding<br> <br>
with raises_sanlock_errno():<br>
sanlock.release(b"ls_name", name, disks)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_release_resource_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.release(b"ls_name", b"res_name", [(path, \
0)])<br> <br>
@@ -596,11 +594,10 @@ def test_read_resource_owners_parse_args(no_sanlock_daemon, \
name, filename, enco<br> <br>
with raises_sanlock_errno():<br>
sanlock.read_resource_owners(b"ls_name", name, disks)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_read_resource_owners_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.read_resource_owners(b"ls_name", \
b"res_name", [(path, 0)])<br> <br>
@@ -658,11 +655,10 @@ def test_init_resource_parse_args(no_sanlock_daemon, name, \
filename, encoding):<br> sanlock.init_resource(b"ls_name", name, \
disks)<br> with raises_sanlock_errno(errno.ENOENT):<br>
sanlock.init_resource(name, b"res_name", disks)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_init_resource_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.init_resource(b"ls_name", b"res_name", \
[(path, 0)])<br> <br>
@@ -689,11 +685,10 @@ def test_read_resource_parse_args(no_sanlock_daemon, filename, \
encoding):<br>
path = util.generate_path("/tmp/", filename, encoding)<br>
with raises_sanlock_errno():<br>
sanlock.read_resource(path)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_read_resource_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.read_resource(path)<br>
<br>
@@ -713,11 +708,10 @@ def test_request_parse_args(no_sanlock_daemon, name, filename, \
encoding):<br> <br>
with raises_sanlock_errno():<br>
sanlock.request(name, b"res_name", disks)<br>
<br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_request_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.request(b"ls_name", b"res_name", [(path, \
0)])<br> <br>
@@ -737,11 +731,10 @@ def test_acquire_parse_args(no_sanlock_daemon, name, filename, \
encoding):<br> <br>
with raises_sanlock_errno():<br>
sanlock.acquire(name, b"res_name", disks, \
pid=os.getpid())<br> <br>
<br>
-@pytest.mark.xfail(reason="path truncated silently")<br>
def test_acquire_path_length(no_sanlock_daemon):<br>
with pytest.raises(ValueError):<br>
path = "x" * constants.SANLK_PATH_LEN<br>
sanlock.acquire(b"ls_name", b"res_name", [(path, \
0)], pid=os.getpid())<br> <br>
-- <br>
2.17.2<br>
<br>
</blockquote></div>
</blockquote></div></div>
</blockquote></div>
[Attachment #6 (text/plain)]
_______________________________________________
sanlock-devel mailing list -- sanlock-devel@lists.fedorahosted.org
To unsubscribe send an email to sanlock-devel-leave@lists.fedorahosted.org
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedorahosted.org/archives/list/sanlock-devel@lists.fedorahosted.org
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic