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