* Segmentation fault due to double free for archetype.
@ 2022-01-15 10:20 Takashi Yano
2022-01-17 11:01 ` Corinna Vinschen
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Yano @ 2022-01-15 10:20 UTC (permalink / raw)
To: cygwin
Hi,
I found the following test case causes segmentation fault
in 32 bit cygwin.
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
int main() {
for (int i = 0; i < 256; i++) {
printf("\r%d, %d\n", i, open("/dev/ptmx", O_RDWR | O_NOCTTY));
}
return 0;
}
The test case results in:
$ ./a.exe
0, 3
1, 4
2, 5
[...]
125, 128
126, 129
0 [main] a 50 tty_list::allocate: No pty allocated
127, -1
1549 [main] a 50 tty_list::allocate: No pty allocated
128, -1
3047 [main] a 50 tty_list::allocate: No pty allocated
129, -1
4625 [main] a 50 tty_list::allocate: No pty allocated
130, -1
6477 [main] a 50 tty_list::allocate: No pty allocated
Segmentation fault (core dumped)
I looked into this problem and found that this is due to
free'ing archetype which was already free'ed by _cfree().
The mechanism of the problem is:
1) archetype is added to archetypes[] at line 675 in dtable.cc
when trying to open pty.
2) Opening pty fails because too many ptys are opened.
3) archetype is deleted at line 444 in fhandler.cc.
4) archetype is copied from archetypes[] at line 659 in dtable.cc
which is already free'ed in step 3) when trying to open pty again.
5) Opening pty fails again.
6) archetype which was already free'ed in step 3) is deleted at
line 444 in fhandler.cc.
I am not sure why this does not happen in 64 bit cygwin.
I guess double free does not cause segfault by chance in
64 bit cygwin.
I also found the following patch fixes the issue. Is this the
right thing?
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index fc7c0422e..e51208117 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -441,7 +441,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
|| open (flags, mode & 07777)))
{
if (archetype)
- delete archetype;
+ cygheap->fdtab.delete_archetype (archetype);
}
else if (archetype)
{
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Segmentation fault due to double free for archetype.
2022-01-15 10:20 Segmentation fault due to double free for archetype Takashi Yano
@ 2022-01-17 11:01 ` Corinna Vinschen
2022-01-17 11:41 ` Takashi Yano
0 siblings, 1 reply; 6+ messages in thread
From: Corinna Vinschen @ 2022-01-17 11:01 UTC (permalink / raw)
To: cygwin
On Jan 15 19:20, Takashi Yano wrote:
> Hi,
>
> I found the following test case causes segmentation fault
> in 32 bit cygwin.
> [...]
> I looked into this problem and found that this is due to
> free'ing archetype which was already free'ed by _cfree().
>
> The mechanism of the problem is:
> 1) archetype is added to archetypes[] at line 675 in dtable.cc
> when trying to open pty.
> 2) Opening pty fails because too many ptys are opened.
> 3) archetype is deleted at line 444 in fhandler.cc.
> 4) archetype is copied from archetypes[] at line 659 in dtable.cc
> which is already free'ed in step 3) when trying to open pty again.
> 5) Opening pty fails again.
> 6) archetype which was already free'ed in step 3) is deleted at
> line 444 in fhandler.cc.
>
> I am not sure why this does not happen in 64 bit cygwin.
> I guess double free does not cause segfault by chance in
> 64 bit cygwin.
>
> I also found the following patch fixes the issue. Is this the
> right thing?
>
> diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
> index fc7c0422e..e51208117 100644
> --- a/winsup/cygwin/fhandler.cc
> +++ b/winsup/cygwin/fhandler.cc
> @@ -441,7 +441,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
> || open (flags, mode & 07777)))
> {
> if (archetype)
> - delete archetype;
> + cygheap->fdtab.delete_archetype (archetype);
> }
> else if (archetype)
> {
Good catch! I think this is basically ok, but you have to check the
usecount, i. e.
if (archetype && archetype_usecount (-1) == 0)
cygheap->fdtab.delete_archetype (archetype);
Does that sound right?
Corinna
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Segmentation fault due to double free for archetype.
2022-01-17 11:01 ` Corinna Vinschen
@ 2022-01-17 11:41 ` Takashi Yano
2022-01-17 12:11 ` Corinna Vinschen
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Yano @ 2022-01-17 11:41 UTC (permalink / raw)
To: cygwin
On Mon, 17 Jan 2022 12:01:51 +0100
Corinna Vinschen wrote:
> On Jan 15 19:20, Takashi Yano wrote:
> > I also found the following patch fixes the issue. Is this the
> > right thing?
> >
> > diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
> > index fc7c0422e..e51208117 100644
> > --- a/winsup/cygwin/fhandler.cc
> > +++ b/winsup/cygwin/fhandler.cc
> > @@ -441,7 +441,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
> > || open (flags, mode & 07777)))
> > {
> > if (archetype)
> > - delete archetype;
> > + cygheap->fdtab.delete_archetype (archetype);
> > }
> > else if (archetype)
> > {
>
> Good catch! I think this is basically ok, but you have to check the
> usecount, i. e.
>
> if (archetype && archetype_usecount (-1) == 0)
> cygheap->fdtab.delete_archetype (archetype);
>
> Does that sound right?
Where is archetype->usecount is incremented? It seems that
archetype->usecount is zero here. archetype->usecount is not
incremented around line 672 in dtable.cc when archetype is
created by fh->clone().
else
{
if (!fh->get_name ())
fh->set_name (fh->dev ().native ());
fh->archetype = fh->clone ();
debug_printf ("created an archetype (%p) for %s(%d/%d)", fh->archetype, fh->get_name (), fh->dev ().get_major (), fh->dev ().get_minor ());
fh->archetype->archetype = NULL;
*cygheap->fdtab.add_archetype () = fh->archetype;
}
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Segmentation fault due to double free for archetype.
2022-01-17 11:41 ` Takashi Yano
@ 2022-01-17 12:11 ` Corinna Vinschen
2022-01-17 12:48 ` Takashi Yano
0 siblings, 1 reply; 6+ messages in thread
From: Corinna Vinschen @ 2022-01-17 12:11 UTC (permalink / raw)
To: cygwin
On Jan 17 20:41, Takashi Yano wrote:
> On Mon, 17 Jan 2022 12:01:51 +0100
> Corinna Vinschen wrote:
> > On Jan 15 19:20, Takashi Yano wrote:
> > > I also found the following patch fixes the issue. Is this the
> > > right thing?
> > >
> > > diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
> > > index fc7c0422e..e51208117 100644
> > > --- a/winsup/cygwin/fhandler.cc
> > > +++ b/winsup/cygwin/fhandler.cc
> > > @@ -441,7 +441,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
> > > || open (flags, mode & 07777)))
> > > {
> > > if (archetype)
> > > - delete archetype;
> > > + cygheap->fdtab.delete_archetype (archetype);
> > > }
> > > else if (archetype)
> > > {
> >
> > Good catch! I think this is basically ok, but you have to check the
> > usecount, i. e.
> >
> > if (archetype && archetype_usecount (-1) == 0)
> > cygheap->fdtab.delete_archetype (archetype);
> >
> > Does that sound right?
>
> Where is archetype->usecount is incremented?
In fhandler_base::open_with_arch.
> It seems that
> archetype->usecount is zero here. archetype->usecount is not
> incremented around line 672 in dtable.cc when archetype is
> created by fh->clone().
>
> else
> {
> if (!fh->get_name ())
> fh->set_name (fh->dev ().native ());
> fh->archetype = fh->clone ();
> debug_printf ("created an archetype (%p) for %s(%d/%d)", fh->archetype, fh->get_name (), fh->dev ().get_major (), fh->dev ().get_minor ());
> fh->archetype->archetype = NULL;
> *cygheap->fdtab.add_archetype () = fh->archetype;
> }
Right, but if open isn't called, because you already have an archetype
at this point:
if (!(res = (archetype && archetype->io_handle)
|| open (flags, mode & 07777)))
Then the archetype is one already created by a former open_with_arch
call and then you delete an archetype which is still in use, no?
Corinna
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Segmentation fault due to double free for archetype.
2022-01-17 12:11 ` Corinna Vinschen
@ 2022-01-17 12:48 ` Takashi Yano
2022-01-17 15:47 ` Corinna Vinschen
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Yano @ 2022-01-17 12:48 UTC (permalink / raw)
To: cygwin
On Mon, 17 Jan 2022 13:11:46 +0100
Corinna Vinschen wrote:
> On Jan 17 20:41, Takashi Yano wrote:
> > On Mon, 17 Jan 2022 12:01:51 +0100
> > Corinna Vinschen wrote:
> > > On Jan 15 19:20, Takashi Yano wrote:
> > > > I also found the following patch fixes the issue. Is this the
> > > > right thing?
> > > >
> > > > diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
> > > > index fc7c0422e..e51208117 100644
> > > > --- a/winsup/cygwin/fhandler.cc
> > > > +++ b/winsup/cygwin/fhandler.cc
> > > > @@ -441,7 +441,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
> > > > || open (flags, mode & 07777)))
> > > > {
> > > > if (archetype)
> > > > - delete archetype;
> > > > + cygheap->fdtab.delete_archetype (archetype);
> > > > }
> > > > else if (archetype)
> > > > {
> > >
> > > Good catch! I think this is basically ok, but you have to check the
> > > usecount, i. e.
> > >
> > > if (archetype && archetype_usecount (-1) == 0)
> > > cygheap->fdtab.delete_archetype (archetype);
> > >
> > > Does that sound right?
> >
> > Where is archetype->usecount is incremented?
>
> In fhandler_base::open_with_arch.
>
> > It seems that
> > archetype->usecount is zero here. archetype->usecount is not
> > incremented around line 672 in dtable.cc when archetype is
> > created by fh->clone().
> >
> > else
> > {
> > if (!fh->get_name ())
> > fh->set_name (fh->dev ().native ());
> > fh->archetype = fh->clone ();
> > debug_printf ("created an archetype (%p) for %s(%d/%d)", fh->archetype, fh->get_name (), fh->dev ().get_major (), fh->dev ().get_minor ());
> > fh->archetype->archetype = NULL;
> > *cygheap->fdtab.add_archetype () = fh->archetype;
> > }
>
> Right, but if open isn't called, because you already have an archetype
> at this point:
>
> if (!(res = (archetype && archetype->io_handle)
> || open (flags, mode & 07777)))
>
> Then the archetype is one already created by a former open_with_arch
> call and then you delete an archetype which is still in use, no?
archetype->usecount is not incremented yet here.
archetype->usecount is incremented only when this 'if' clause
is not true. So, isn't the following code right?
if (!(res = (archetype && archetype->io_handle)
|| open (flags, mode & 07777)))
{
if (archetype && archetype->usecount == 0)
cygheap->fdtab.delete_archetype (archetype);
}
--
Takashi Yano <takashi.yano@nifty.ne.jp>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Segmentation fault due to double free for archetype.
2022-01-17 12:48 ` Takashi Yano
@ 2022-01-17 15:47 ` Corinna Vinschen
0 siblings, 0 replies; 6+ messages in thread
From: Corinna Vinschen @ 2022-01-17 15:47 UTC (permalink / raw)
To: cygwin
On Jan 17 21:48, Takashi Yano wrote:
> On Mon, 17 Jan 2022 13:11:46 +0100
> Corinna Vinschen wrote:
> > if (!(res = (archetype && archetype->io_handle)
> > || open (flags, mode & 07777)))
> >
> > Then the archetype is one already created by a former open_with_arch
> > call and then you delete an archetype which is still in use, no?
>
> archetype->usecount is not incremented yet here.
> archetype->usecount is incremented only when this 'if' clause
> is not true. So, isn't the following code right?
>
> if (!(res = (archetype && archetype->io_handle)
> || open (flags, mode & 07777)))
> {
> if (archetype && archetype->usecount == 0)
> cygheap->fdtab.delete_archetype (archetype);
> }
Good point, yes, that sounds right to me.
Thanks,
Corinna
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-17 15:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-15 10:20 Segmentation fault due to double free for archetype Takashi Yano
2022-01-17 11:01 ` Corinna Vinschen
2022-01-17 11:41 ` Takashi Yano
2022-01-17 12:11 ` Corinna Vinschen
2022-01-17 12:48 ` Takashi Yano
2022-01-17 15:47 ` Corinna Vinschen
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).