* [PATCH] bug in emergency cxa pool free()
@ 2022-08-16 19:14 Keef Aragon
2022-08-17 6:45 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Keef Aragon @ 2022-08-16 19:14 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
This probably has never actually affected anyone in practice. The normal
ABI implementation just uses malloc and only falls back to the pool on
malloc failure. But if that happens a bunch of times the freelist gets out
of order which violates some of the invariants of the freelist (as well as
the comments that follow the bug). The bug is just a comparison reversal
when traversing the freelist in the case where the pointer being returned
to the pool is after the existing freelist.
I'm not sure what to do as far as the test suite is concerned. It's a
private part of the implementation of the exception handling ABI and it can
only ever be triggered if malloc fails (repeatedly). So it seems like
reproducing it from the external interface will require hooking malloc to
forcibly return NULL.
But I'm a newb on these lists, so will obediently do as instructed.
[-- Attachment #2: eh_alloc.patch --]
[-- Type: text/x-patch, Size: 858 bytes --]
diff --git a/libstdc++-v3/ChangeLog-2022 b/libstdc++-v3/ChangeLog-2022
new file mode 100644
index 00000000000..8057de58539
--- /dev/null
+++ b/libstdc++-v3/ChangeLog-2022
@@ -0,0 +1,4 @@
+2022-08-16 Keef Aragon <keef.aragon@konscious.net>
+
+ * libstdc++-v3/libsupc++/eh_alloc.cc: inverse comparison in pool::free
+
diff --git a/libstdc++-v3/libsupc++/eh_alloc.cc b/libstdc++-v3/libsupc++/eh_alloc.cc
index c85b9aed40b..cad2750e3b9 100644
--- a/libstdc++-v3/libsupc++/eh_alloc.cc
+++ b/libstdc++-v3/libsupc++/eh_alloc.cc
@@ -225,7 +225,7 @@ namespace
for (fe = &first_free_entry;
(*fe)->next
&& (reinterpret_cast <char *> ((*fe)->next)
- > reinterpret_cast <char *> (e) + sz);
+ < reinterpret_cast <char *> (e) + sz);
fe = &(*fe)->next)
;
// If we can merge the next block into us do so and continue
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bug in emergency cxa pool free()
2022-08-16 19:14 [PATCH] bug in emergency cxa pool free() Keef Aragon
@ 2022-08-17 6:45 ` Richard Biener
2022-08-17 19:02 ` Keef Aragon
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2022-08-17 6:45 UTC (permalink / raw)
To: Keef Aragon; +Cc: libstdc++, GCC Patches, Jonathan Wakely
On Tue, Aug 16, 2022 at 9:15 PM Keef Aragon <keef.aragon@konscious.net> wrote:
>
> This probably has never actually affected anyone in practice. The normal
> ABI implementation just uses malloc and only falls back to the pool on
> malloc failure. But if that happens a bunch of times the freelist gets out
> of order which violates some of the invariants of the freelist (as well as
> the comments that follow the bug). The bug is just a comparison reversal
> when traversing the freelist in the case where the pointer being returned
> to the pool is after the existing freelist.
>
> I'm not sure what to do as far as the test suite is concerned. It's a
> private part of the implementation of the exception handling ABI and it can
> only ever be triggered if malloc fails (repeatedly). So it seems like
> reproducing it from the external interface will require hooking malloc to
> forcibly return NULL.
>
> But I'm a newb on these lists, so will obediently do as instructed.
Oops, that's my fault.
For consistency it's probably best written
as reinterpret_cast <char *> (e) + sz > reinterpret_cast <char *> ((*fe))
thus
diff --git a/libstdc++-v3/libsupc++/eh_alloc.cc
b/libstdc++-v3/libsupc++/eh_alloc.cc
index c85b9aed40b..68f319869f9 100644
--- a/libstdc++-v3/libsupc++/eh_alloc.cc
+++ b/libstdc++-v3/libsupc++/eh_alloc.cc
@@ -224,8 +224,8 @@ namespace
free_entry **fe;
for (fe = &first_free_entry;
(*fe)->next
- && (reinterpret_cast <char *> ((*fe)->next)
- > reinterpret_cast <char *> (e) + sz);
+ && (reinterpret_cast <char *> (e) + sz
+ > reinterpret_cast <char *> ((*fe)->next));
fe = &(*fe)->next)
;
// If we can merge the next block into us do so and continue
The change is OK with that adjustment. I see you do not have write access so
I'll test & push it for you.
I'm curious how you noticed?
Thanks,
Richard.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bug in emergency cxa pool free()
2022-08-17 6:45 ` Richard Biener
@ 2022-08-17 19:02 ` Keef Aragon
0 siblings, 0 replies; 3+ messages in thread
From: Keef Aragon @ 2022-08-17 19:02 UTC (permalink / raw)
To: Richard Biener; +Cc: libstdc++, GCC Patches, Jonathan Wakely
Thank you!
I was working on a modified version that I could LD_PRELOAD to investigate
some issues I was having in my asynchronous application code. It used a
higher order collection of pool instances instead of malloc/free, with
extra context (an array of pc register values) packed into the buffer
before the refcounted exception. in_pool on the pools in the collection is
the safety check to know that the extra context can be safely pulled from
the pointer.
With that implementation, memory was being leaked because the destruction
of one of the pools in the collection requires there to be no more uses of
it and quickly checking for that to be the case also needed the ordered
invariant to hold (just checking if the freelist the whole arena).
On Tue, Aug 16, 2022 at 11:45 PM Richard Biener <richard.guenther@gmail.com>
wrote:
> On Tue, Aug 16, 2022 at 9:15 PM Keef Aragon <keef.aragon@konscious.net>
> wrote:
> >
> > This probably has never actually affected anyone in practice. The normal
> > ABI implementation just uses malloc and only falls back to the pool on
> > malloc failure. But if that happens a bunch of times the freelist gets
> out
> > of order which violates some of the invariants of the freelist (as well
> as
> > the comments that follow the bug). The bug is just a comparison reversal
> > when traversing the freelist in the case where the pointer being returned
> > to the pool is after the existing freelist.
> >
> > I'm not sure what to do as far as the test suite is concerned. It's a
> > private part of the implementation of the exception handling ABI and it
> can
> > only ever be triggered if malloc fails (repeatedly). So it seems like
> > reproducing it from the external interface will require hooking malloc to
> > forcibly return NULL.
> >
> > But I'm a newb on these lists, so will obediently do as instructed.
>
> Oops, that's my fault.
> For consistency it's probably best written
> as reinterpret_cast <char *> (e) + sz > reinterpret_cast <char *> ((*fe))
> thus
>
> diff --git a/libstdc++-v3/libsupc++/eh_alloc.cc
> b/libstdc++-v3/libsupc++/eh_alloc.cc
> index c85b9aed40b..68f319869f9 100644
> --- a/libstdc++-v3/libsupc++/eh_alloc.cc
> +++ b/libstdc++-v3/libsupc++/eh_alloc.cc
> @@ -224,8 +224,8 @@ namespace
> free_entry **fe;
> for (fe = &first_free_entry;
> (*fe)->next
> - && (reinterpret_cast <char *> ((*fe)->next)
> - > reinterpret_cast <char *> (e) + sz);
> + && (reinterpret_cast <char *> (e) + sz
> + > reinterpret_cast <char *> ((*fe)->next));
> fe = &(*fe)->next)
> ;
> // If we can merge the next block into us do so and continue
>
> The change is OK with that adjustment. I see you do not have write access
> so
> I'll test & push it for you.
>
> I'm curious how you noticed?
>
> Thanks,
> Richard.
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-17 19:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 19:14 [PATCH] bug in emergency cxa pool free() Keef Aragon
2022-08-17 6:45 ` Richard Biener
2022-08-17 19:02 ` Keef Aragon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).