Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
On 10.04.2018 13:04, Raman Shishniou wrote:
Hello,
On 04/10/2018 01:38 PM, Georg Chini wrote:
On 10.04.2018 10:21, Raman Shishniou wrote:
Hello,
On 04/09/2018 07:15 PM, Georg Chini wrote:
sco_process_render does not unref the memblock when it encounters an error.
This patch fixes the issue. It also changes the return value to 1 in the case
of EAGAIN. Because the data was already rendered and cannot be re-sent, we
have to discard the block.
---
src/modules/bluetooth/module-bluez5-device.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 95d288ef..b81c233c 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
if (errno == EINTR)
/* Retry right away if we got interrupted */
continue;
-else if (errno == EAGAIN)
-/* Hmm, apparently the socket was not writable, give up for now */
-return 0;
+
+pa_memblock_unref(memchunk.memblock);
+
+if (errno == EAGAIN)
+/* Hmm, apparently the socket was not writable, give up for now.
+ * Because the data was already rendered, let's discard the block.
*/
+return 1;
1. errno value can be changed during pa_memblock_unref()
I don't think so. The only possible system calls used during unref should be
calls to free() and because we always use pa_xfree(), the errno value will
be preserved.
I seen callback processing during memblock_free()
You are right, b->per_type.user.free_cb() is a user defined function
which could do anything. I'll send an updated version.
2. I think the same changes are required for a2dp_process_render() too.
No, a2dp_process_render() works slightly different. It keeps the memchunk
in userdata and tries to re-send the same block again.
pa_log_error("Failed to write data to SCO socket: %s",
pa_cstrerror(errno));
return -1;
@@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
pa_log_error("Wrote memory block to socket only partially! %llu written,
wanted to write %llu.",
(unsigned long long) l,
(unsigned long long) memchunk.length);
+
+pa_memblock_unref(memchunk.memblock);
return -1;
}
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
Hello,
On 04/10/2018 01:38 PM, Georg Chini wrote:
> On 10.04.2018 10:21, Raman Shishniou wrote:
>> Hello,
>>
>> On 04/09/2018 07:15 PM, Georg Chini wrote:
>>> sco_process_render does not unref the memblock when it encounters an error.
>>>
>>> This patch fixes the issue. It also changes the return value to 1 in the
>>> case
>>> of EAGAIN. Because the data was already rendered and cannot be re-sent, we
>>> have to discard the block.
>>> ---
>>> src/modules/bluetooth/module-bluez5-device.c | 12 +---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/modules/bluetooth/module-bluez5-device.c
>>> b/src/modules/bluetooth/module-bluez5-device.c
>>> index 95d288ef..b81c233c 100644
>>> --- a/src/modules/bluetooth/module-bluez5-device.c
>>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>>> @@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
>>> if (errno == EINTR)
>>> /* Retry right away if we got interrupted */
>>> continue;
>>> -else if (errno == EAGAIN)
>>> -/* Hmm, apparently the socket was not writable, give up for
>>> now */
>>> -return 0;
>>> +
>>> +pa_memblock_unref(memchunk.memblock);
>>> +
>>> +if (errno == EAGAIN)
>>> +/* Hmm, apparently the socket was not writable, give up for
>>> now.
>>> + * Because the data was already rendered, let's discard the
>>> block. */
>>> +return 1;
>>>
>> 1. errno value can be changed during pa_memblock_unref()
>
> I don't think so. The only possible system calls used during unref should be
> calls to free() and because we always use pa_xfree(), the errno value will
> be preserved.
>
I seen callback processing during memblock_free()
>> 2. I think the same changes are required for a2dp_process_render() too.
>
> No, a2dp_process_render() works slightly different. It keeps the memchunk
> in userdata and tries to re-send the same block again.
>
>>
>>
>>> pa_log_error("Failed to write data to SCO socket: %s",
>>> pa_cstrerror(errno));
>>> return -1;
>>> @@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
>>> pa_log_error("Wrote memory block to socket only partially! %llu
>>> written, wanted to write %llu.",
>>> (unsigned long long) l,
>>> (unsigned long long) memchunk.length);
>>> +
>>> +pa_memblock_unref(memchunk.memblock);
>>> return -1;
>>> }
>>>
>> --
>> Raman
>
>
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
On 10.04.2018 10:21, Raman Shishniou wrote:
Hello,
On 04/09/2018 07:15 PM, Georg Chini wrote:
sco_process_render does not unref the memblock when it encounters an error.
This patch fixes the issue. It also changes the return value to 1 in the case
of EAGAIN. Because the data was already rendered and cannot be re-sent, we
have to discard the block.
---
src/modules/bluetooth/module-bluez5-device.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 95d288ef..b81c233c 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
if (errno == EINTR)
/* Retry right away if we got interrupted */
continue;
-else if (errno == EAGAIN)
-/* Hmm, apparently the socket was not writable, give up for now */
-return 0;
+
+pa_memblock_unref(memchunk.memblock);
+
+if (errno == EAGAIN)
+/* Hmm, apparently the socket was not writable, give up for now.
+ * Because the data was already rendered, let's discard the block.
*/
+return 1;
1. errno value can be changed during pa_memblock_unref()
I don't think so. The only possible system calls used during unref should be
calls to free() and because we always use pa_xfree(), the errno value will
be preserved.
2. I think the same changes are required for a2dp_process_render() too.
No, a2dp_process_render() works slightly different. It keeps the memchunk
in userdata and tries to re-send the same block again.
pa_log_error("Failed to write data to SCO socket: %s",
pa_cstrerror(errno));
return -1;
@@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
pa_log_error("Wrote memory block to socket only partially! %llu written,
wanted to write %llu.",
(unsigned long long) l,
(unsigned long long) memchunk.length);
+
+pa_memblock_unref(memchunk.memblock);
return -1;
}
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
Hello,
On 04/09/2018 07:15 PM, Georg Chini wrote:
> sco_process_render does not unref the memblock when it encounters an error.
>
> This patch fixes the issue. It also changes the return value to 1 in the case
> of EAGAIN. Because the data was already rendered and cannot be re-sent, we
> have to discard the block.
> ---
> src/modules/bluetooth/module-bluez5-device.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/modules/bluetooth/module-bluez5-device.c
> b/src/modules/bluetooth/module-bluez5-device.c
> index 95d288ef..b81c233c 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
> if (errno == EINTR)
> /* Retry right away if we got interrupted */
> continue;
> -else if (errno == EAGAIN)
> -/* Hmm, apparently the socket was not writable, give up for now
> */
> -return 0;
> +
> +pa_memblock_unref(memchunk.memblock);
> +
> +if (errno == EAGAIN)
> +/* Hmm, apparently the socket was not writable, give up for now.
> + * Because the data was already rendered, let's discard the
> block. */
> +return 1;
>
1. errno value can be changed during pa_memblock_unref()
2. I think the same changes are required for a2dp_process_render() too.
> pa_log_error("Failed to write data to SCO socket: %s",
> pa_cstrerror(errno));
> return -1;
> @@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
> pa_log_error("Wrote memory block to socket only partially! %llu
> written, wanted to write %llu.",
> (unsigned long long) l,
> (unsigned long long) memchunk.length);
> +
> +pa_memblock_unref(memchunk.memblock);
> return -1;
> }
>
>
--
Raman
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] bluez5-device: Fix memory leak in sco_process_render()
sco_process_render does not unref the memblock when it encounters an error.
This patch fixes the issue. It also changes the return value to 1 in the case
of EAGAIN. Because the data was already rendered and cannot be re-sent, we
have to discard the block.
---
src/modules/bluetooth/module-bluez5-device.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 95d288ef..b81c233c 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -282,9 +282,13 @@ static int sco_process_render(struct userdata *u) {
if (errno == EINTR)
/* Retry right away if we got interrupted */
continue;
-else if (errno == EAGAIN)
-/* Hmm, apparently the socket was not writable, give up for now */
-return 0;
+
+pa_memblock_unref(memchunk.memblock);
+
+if (errno == EAGAIN)
+/* Hmm, apparently the socket was not writable, give up for now.
+ * Because the data was already rendered, let's discard the block.
*/
+return 1;
pa_log_error("Failed to write data to SCO socket: %s",
pa_cstrerror(errno));
return -1;
@@ -296,6 +300,8 @@ static int sco_process_render(struct userdata *u) {
pa_log_error("Wrote memory block to socket only partially! %llu
written, wanted to write %llu.",
(unsigned long long) l,
(unsigned long long) memchunk.length);
+
+pa_memblock_unref(memchunk.memblock);
return -1;
}
--
2.14.1
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
