Repository: sabnzbd/sabctools (language: python)

Issue:
Buffer based approach, second try
@animetosho If you have time, could you maybe check if my new attempt has dangerous points in it or if I made conceptual errors?

Thanks so much :)

@mnightingale FYI!


Discussion / hints:
From a quick look:

* Recommend using `my_memstr` over `strstr`, to avoid scanning past `data_length`
* Recommend searching for `" name="` instead of just `"name="`, just in case someone decides to stick some other name in the line in the future
* dereferencing `cur_char` in address calculations is likely incorrect. `*cur_char` means 'the *character* at memory location `cur_char`', which is going to be unrelated to a memory address.  
You'll also need to consider that `cur_char + data_length` will point past the end of the data. I'm guessing what you actually want is a pointer to the end of the data, and compute based off that.

For example:

	start_loc = my_memstr(cur_char, data_length - *cur_char, "=ypart ", 1);

You probably want something like this instead:

	char *data_end = dest_loc + data_length;
	start_loc = my_memstr(cur_char, data_end - cur_char, "=ypart ", 1);
	// `data_end - cur_char` might be too long though, as there's no need to search the entire article for the marker - perhaps consider limiting the search range

I don't quite get the [data_step logic](https://github.com/sabnzbd/sabyenc/pull/79/files#diff-507abba6877e4f263d778a243063a187e86588da0259e065cbbd4b8158c02e24R658-R661). I'm guessing it should be:

	if (data_end - MAX_TAIL_BYTES > cur_char) {
		cur_char = data_end - MAX_TAIL_BYTES;
	}

Otherwise it looks neat 👍
Thanks so much for your feedback, I feel stupid using those pointers wrongly.
If you have the time to take a last look for bad code from this C-novice?
@puzzledsab, if you re-install sabyenc3 I hope the segfaults are over now.
@mnightingale I checked the [cpython source](https://github.com/python/cpython/blob/main/Include/ceval.h#L98-L107) and saw that we can actually lift the GIL earlier by using `Py_BLOCK_THREADS` in the error cases. 
Looks good!  
I'm guessing the scan [here](https://github.com/sabnzbd/sabyenc/pull/79/files#diff-507abba6877e4f263d778a243063a187e86588da0259e065cbbd4b8158c02e24R636) should actually be `my_memstr(start_loc, end_loc - start_loc, " name=", 1);`
I do think a read/write buffer should be used for the input/output [Strings and buffers](https://docs.python.org/3/c-api/arg.html#strings-and-buffers) - I think it's possible what's happening now could cause stability issues (it's different to what decode chunks did since it appear it just read from the bytes and wrote to a new object)

> Formats such as y* and s* fill a [Py_buffer](https://docs.python.org/3/c-api/buffer.html#c.Py_buffer) structure. This locks the underlying buffer so that the caller can subsequently use the buffer even inside a [Py_BEGIN_ALLOW_THREADS](https://docs.python.org/3/c-api/init.html#c.Py_BEGIN_ALLOW_THREADS) block without the risk of mutable data being resized or destroyed. As a result, you have to call [PyBuffer_Release()](https://docs.python.org/3/c-api/buffer.html#c.PyBuffer_Release) after you have finished processing the data (or in any early abort case).

Therefore I think a bytearray should be passed in, get a buffer lock on it, release the lock and resize.
It makes it safe to write, and the type isn't supposed to be read only.
The buffer from bytes doesn't lock (it doesn't increase ob_exports because it's readonly) - further information on this [here](https://eli.thegreenplace.net/2011/11/28/less-copies-in-python-with-the-buffer-protocol-and-memoryviews)

POC https://gist.github.com/mnightingale/3e886c5ace7550daad65718c93099ae3
This works, I don't think we are allocating anymore memory, its safer.
You'll probably want other changes:
- Support your resize method - it's a shame python doesn't give control over the resizes to avoid it actually resizing memory
- data_length isn't required safer to get it from the buffer and then could use METH_O
- ~~Need to check the flags used for the buffer against what parsetuple would request~~ - I think its fine
- edit: should check return value of PyObject_GetBuffer

Also noticed [this to check](https://github.com/sabnzbd/sabnzbd/blob/ac32d3c835dfc6ef557e4236ed56d8f82fe6c1bc/sabnzbd/decoder.py#L189) is this from when raw_data was a list of chunks not bytes/bytearray?

Further below that does the "Convert the initial chunks of raw socket data to article lines" looks to be dealing with lists/chunks, but my concern was does this happen often? the splitting of lines seems expensive - if its like 0.01% of the time it doesn't matter.

Finally [this and a few other places](https://github.com/sabnzbd/sabnzbd/blob/ac32d3c835dfc6ef557e4236ed56d8f82fe6c1bc/sabnzbd/newswrapper.py#L210) slicing of the underlying buffer self.data which I believe would make copies rather than via the data_view.
I doubt any of the places this occurs make very much difference since its always such small slices/copies (end of article and status code).
Reference for this https://julien.danjou.info/high-performance-in-python-with-zero-copy-and-the-buffer-protocol/



Sounds good. If you create a PR from the patch I can merge it.
~I don't fully understand why the codes uses both buffer and bytes array? Are both needed?~ Edit: I understand now! 

Seems the `PyByteArray_Resize` method does the same as my custom method: https://github.com/python/cpython/blob/main/Objects/bytearrayobject.c#L201

Regarding the checks in decoder: I just didn't convert them yet to check the buffer, similarly to still needing to convert the UU decode to use the buffer instead of chucks. Just wanted to start testing.
Sure I'll take a look at it later or over the weekend.

I'm not even sure I understood all the reason for having bytearray and buffer but I think the main ones are:
- PyByteArray_Resize does a check that ob_exports is not >0 so no one else has an export into the bytearray
  - If it always allowed other users could try and read memory beyond the end - the custom method doesn't do this check
  - Because we should release the buffer before a resize we'd have no reference to the PyObject so I think we need switch it around and take a bytearray and get a buffer temporarily
- Py_buffer is just a safe way to access the internals of objects and can be safe to operate on without the GIL - you have to ensure other GIL'd python code isn't writing to the data from another thread or you'd get strange data changes as a result
- I'm not totally clear but I think operating on PyByteArray directly may not guarantee that the data is contiguous, that is given a start and length buf[0]-buf[length-1] may not be where the data resides, again I'm not really clear on this but of note when requesting a Py_buffer you can explicitly tell it you need contiguous access.

So the basic flow needs to be:
- Pass in bytearray
- Get a buffer and its size
- Release GIL
- do_decode
- Acquire GIL
- Release buffer
- Resize bytearray

We may also be able to make the pointer calculations a bit simpler, it seems we could do buf[0]...buf[length-1] e.g. offsets instead of exact locations.

The custom resize method appear to just ensure that the "Minor downsize; quick exit" path is taken which sets the size and adds a NULL terminator.
I do think `PyByteArray_AS_STRING(self)[size] = '\0';` should be used instead of Python internals `ob_sval` and skipping any check it would do.

With your patch we don't need the custom resize anymore anyway, it's only for the chunks approach and that one will be removed after this is final and doesn't throw any segfaults any mods.

I don't fully understand your suggestion for the pointer math, but look forward to your suggestions! 
It does look like _PyBytes_Resize and PyByteArray_Resize are a bit different, _PyBytes_Resize always reallocated memory whereas PyByteArray_Resize is a little smarter with two possible paths for size reductions:
- the same as your custom function (set size and set null terminator) 
- a slower path which reallocates and copies memory but is only taken if the new size is less than 50% of the currently allocated size

I would be very rare that an article triggers resizing due to being <50% of the raw data size so I think using PyByteArray_Resize would be best - just need to check older Python versions follow this strategy.

I'll probably mostly leave the pointer stuff alone but will probably try and test we can't go past the bounds.
Do you remember how to write pickles in the same format as the others?
I made something that worked but I don't think I got the format quite right.

Also you may find .gitattributes useful, something like `*.pickle text eol=lf`or maybe just binary looks like there have issues over the years.
They were dumps from SABnzbd within the code. Indeed long ago so might just make new ones with different format. They were mostly focused on all that could. Go wrong in the chunked approach, like things being split along multiple chunks. 
I made this which is similar https://gist.github.com/mnightingale/fae581d72913b3861172c1f6d2e12de0 but uses the default binary protocol which was added in 3.0 I think the  old ones are protocol 0.
I think paired with .gitattributes `*.pickle binary` it would avoid some issues but would not be human readable.
I'm not sure what the data_bytes count is but it doesn't appear to be used anymore.

I just wanted to add a decode where a CRC of 0 is valid and check we get 0 and not None returned - it seems to work correctly.

Fix patch:
diff --git a/src/sabyenc3.cc b/src/sabyenc3.cc
index b12fb38..adbb35c 100644
--- a/src/sabyenc3.cc
+++ b/src/sabyenc3.cc
@@ -47,8 +47,8 @@ static PyMethodDef sabyenc3_methods[] = {
     {
         "decode_buffer",
         decode_buffer,
-        METH_VARARGS,
-        "decode_buffer(raw_data, raw_data_size)"
+        METH_O,
+        "decode_buffer(raw_data)"
     },
     {
         "encode",
@@ -577,11 +577,11 @@ PyObject* decode_usenet_chunks(PyObject* self, PyObject* Py_input_list) {
     return retval;
 }
 
-PyObject* decode_buffer(PyObject* self, PyObject* args) {
+PyObject* decode_buffer(PyObject* self, PyObject* Py_bytesarray_obj) {
     // The input/output PyObjects
     (void)self;
     PyObject *retval = NULL;
-    PyBytesObject *Py_bytes_obj = NULL;
+    Py_buffer Py_buffer_obj;
     PyObject *Py_output_filename = NULL;
     PyObject *Py_output_crc = NULL;
     int data_length;
@@ -597,15 +597,22 @@ PyObject* decode_buffer(PyObject* self, PyObject* args) {
     size_t output_len;
     const char* crc_pos;
 
-    // Parse input
-    if (!PyArg_ParseTuple(args, "Si:decode", &Py_bytes_obj, &data_length)) {
+    // Verify it's a bytearray
+    if (!PyByteArray_Check(Py_bytesarray_obj)) {
+        PyErr_SetString(PyExc_TypeError, "Expected bytearray");        
+        return NULL;
+    }
+
+    if (PyObject_GetBuffer(Py_bytesarray_obj, &Py_buffer_obj, PyBUF_CONTIG) != 0) {
         return NULL;
     }
-    dest_loc = cur_char = (char *)Py_bytes_obj->ob_sval;
-    end_loc = dest_loc + data_length;
+
+    dest_loc = cur_char = (char*)Py_buffer_obj.buf;
+    end_loc = dest_loc + Py_buffer_obj.len;
+    output_len = Py_buffer_obj.len;
 
     // Check for valid size
-    if(data_length <= 0 || data_length > Py_SIZE(Py_bytes_obj)) {
+    if (Py_buffer_obj.len <= 0) {
         PyErr_SetString(PyExc_ValueError, "Invalid data length");
         retval = NULL;
         goto finish;
@@ -700,9 +707,6 @@ PyObject* decode_buffer(PyObject* self, PyObject* args) {
     // Return GIL to perform Python modifications
     Py_END_ALLOW_THREADS;
 
-    // Terminate buffer and adjust the Python-size of the bytes-object
-    resize_pybytes(Py_bytes_obj, output_len);
-
     // Is there a valid CRC?
     if (crc != crc_yenc) {
         Py_output_crc = Py_None;
@@ -715,7 +719,12 @@ PyObject* decode_buffer(PyObject* self, PyObject* args) {
     retval = Py_BuildValue("(S, N)", Py_output_filename, Py_output_crc);
 
 finish:
-    if(Py_output_filename) Py_XDECREF(Py_output_filename);
+    Py_XDECREF(Py_output_filename);
+
+    // Terminate buffer and adjust the Python-size of the bytes-object
+    PyBuffer_Release(&Py_buffer_obj);
+    PyByteArray_Resize(Py_bytesarray_obj, output_len);
+    
     return retval;
 }
 


Test patch:
diff --git a/tests/test_decoder.py b/tests/test_decoder.py
index a485479..74b6b45 100644
--- a/tests/test_decoder.py
+++ b/tests/test_decoder.py
@@ -46,7 +46,7 @@ def test_no_filename():
 
 def test_bad_size():
     with pytest.raises(ValueError) as excinfo:
-        sabyenc3.decode_buffer(b"1234", 10)
+        sabyenc3.decode_buffer(bytearray())
     assert "Invalid data length" in str(excinfo.value)
 
 
diff --git a/tests/testsupport.py b/tests/testsupport.py
index acfcf01..0ca5de1 100644
--- a/tests/testsupport.py
+++ b/tests/testsupport.py
@@ -49,9 +49,9 @@ def correct_unknown_encoding(str_or_bytes_in):
             return str_or_bytes_in.decode(chardet.detect(str_or_bytes_in)["encoding"])
 
 
-def read_plain_yenc_file(filename: str) -> bytes:
+def read_plain_yenc_file(filename: str) -> bytearray:
     with open("tests/yencfiles/%s" % filename, "rb") as yencfile:
-        return yencfile.read()
+        return bytearray(yencfile.read())
 
 
 def read_pickle(filename):
@@ -62,11 +62,11 @@ def read_pickle(filename):
             # Reset the pointer and try again
             yencfile.seek(0)
             data_chunks, data_bytes, lines = pickle.load(yencfile, encoding="bytes")
-    return b"".join(data_chunks)
+    return bytearray(b"".join(data_chunks))
 
 
-def sabyenc3_wrapper(data: bytes):
-    filename, crc_correct = sabyenc3.decode_buffer(data, len(data))
+def sabyenc3_wrapper(data: bytearray):
+    filename, crc_correct = sabyenc3.decode_buffer(data)
     return data, correct_unknown_encoding(filename), crc_correct