| Message ID | 20210506133758.1749233-1-philmd@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | misc: Replace alloca() by g_malloc() | expand |
On Thu, May 6, 2021 at 7:39 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > The ALLOCA(3) man-page mentions its "use is discouraged". > Replace few calls by equivalent GLib malloc(). > Except g_alloc and g_malloc are not at all the same, and you can't drop in replace one with the other. g_alloc allocates stack space on the calling frame that's automatically freed when the function returns. g_malloc allocates space from the heap, and calls to it must be matched with calls to g_free(). These patches don't do the latter, as far as I can tell, and so introduce memory leaks unless there's something I've missed. Warner > Last call site is linux-user/. > > Since v1: > - Converted more uses (alsaaudio, tpm, pca9552) > - Reworked gdbstub (Alex) > - Simplified PPC/KVM (Greg) > > Philippe Mathieu-Daudé (9): > audio/alsaaudio: Replace ALSA alloca() by malloc() equivalent > backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD > backends/tpm: Replace g_alloca() by g_malloc() > bsd-user/syscall: Replace alloca() by g_new() > gdbstub: Constify GdbCmdParseEntry > gdbstub: Only call cmd_parse_params() with non-NULL command schema > gdbstub: Replace alloca() + memset(0) by g_new0() > hw/misc/pca9552: Replace g_newa() by g_new() > target/ppc/kvm: Replace alloca() by g_malloc() > > audio/alsaaudio.c | 11 +++++++---- > backends/tpm/tpm_emulator.c | 35 +++++++++++++++-------------------- > bsd-user/syscall.c | 3 +-- > gdbstub.c | 34 +++++++++++++++------------------- > hw/misc/pca9552.c | 2 +- > target/ppc/kvm.c | 3 +-- > 6 files changed, 40 insertions(+), 48 deletions(-) > > -- > 2.26.3 > > > >
On 5/6/21 9:22 AM, Warner Losh wrote: > On Thu, May 6, 2021 at 7:39 AM Philippe Mathieu-Daudé <philmd@redhat.com> > wrote: > >> The ALLOCA(3) man-page mentions its "use is discouraged". >> Replace few calls by equivalent GLib malloc(). >> > > Except g_alloc and g_malloc are not at all the same, and you can't drop in > replace one with the other. > > g_alloc allocates stack space on the calling frame that's automatically > freed when the function returns. > g_malloc allocates space from the heap, and calls to it must be matched > with calls to g_free(). > > These patches don't do the latter, as far as I can tell, and so introduce > memory leaks unless there's something I've missed. You missed the g_autofree, whose job is to call g_free() on all points in the control flow where the malloc()d memory goes out of scope (equivalent in expressive power to alloca()d memory going out of scope). Although the code is arguably a bit slower (as heap manipulations are not as cheap as stack manipulations), in the long run that speed penalty is worth the safety factor (since stack manipulations under user control are inherently unsafe).
On Thu, May 6, 2021 at 8:28 AM Eric Blake <eblake@redhat.com> wrote: > On 5/6/21 9:22 AM, Warner Losh wrote: > > On Thu, May 6, 2021 at 7:39 AM Philippe Mathieu-Daudé <philmd@redhat.com > > > > wrote: > > > >> The ALLOCA(3) man-page mentions its "use is discouraged". > >> Replace few calls by equivalent GLib malloc(). > >> > > > > Except g_alloc and g_malloc are not at all the same, and you can't drop > in > > replace one with the other. > > > > g_alloc allocates stack space on the calling frame that's automatically > > freed when the function returns. > > g_malloc allocates space from the heap, and calls to it must be matched > > with calls to g_free(). > > > > These patches don't do the latter, as far as I can tell, and so introduce > > memory leaks unless there's something I've missed. > > You missed the g_autofree, whose job is to call g_free() on all points > in the control flow where the malloc()d memory goes out of scope > (equivalent in expressive power to alloca()d memory going out of scope). > Although the code is arguably a bit slower (as heap manipulations are > not as cheap as stack manipulations), in the long run that speed penalty > is worth the safety factor (since stack manipulations under user control > are inherently unsafe). > Yea, I had missed that. Too many years of reviewing patches that came in for other projects that didn't use that feature. Outside of the bsd-user context, I'd agree with you that things are safer this way. But emulating system calls has other considerations, that I've gone into in another thread. So please accept my apology for not noticing the g_autofree in these. It definitely makes what I said here incorrect and it should be ignored... Warner