Bug #113390 for Curses: SEGV in Curses (menus)

Sun Mar 27 22:25:58 2016 cpan [...] greatships.com - Ticket created

Subject:	SEGV in Curses (menus)
Date:	Sun, 27 Mar 2016 22:25:38 -0400
To:	bug-Curses [...] rt.cpan.org
From:	LJ McDonald <cpan [...] greatships.com>

Message body is not shown because it is too large.

Message body is not shown because sender requested not to inline it.

Wed Mar 30 17:25:08 2016 bryanh [...] giraffe-data.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	30 Mar 2016 20:50:36 +0000
To:	bug-Curses [...] rt.cpan.org
From:	Bryan Henderson <bryanh [...] giraffe-data.com>

Thank you for figuring that out. This is a pretty basic problem. However, the solution isn't very obvious because of the unusual architecture of the Curses extension. It is designed to be a bare minimum interface to the C Curses library, with the user expected to know about the C objects and explicitly manage the memory (e.g. the user has to call free_menu, whereas in normal Perl, just destroying all the references to the menu would be enough). Based on that, I think the "workaround" is actually the solution that is consistent with the rest of the interface. The example program is wrong, and I will fix that (and the same problem with the forms demo). As noted under "Improvement", this is not a very good interface regardless, and it would be really nice to have a normal Perl interface with the underlying C library behavior hidden. There should be no preserving of variables, freeing, or packing. One should create a menu with Curses::Menu->new(@itemList) where @itemList is an array of references to item objects. And of course, the whole rest of the package should be the same way. With the heavy lifting (translation to C) already done by the existing package and extension, it should not be difficult to create a new Curses2 package in pure Perl that has such a normal Perl interface above and uses the Curses package below to do the work, taking care of all the hackery within. I will look into that. -- Bryan Henderson San Jose, California

Wed Mar 30 17:25:08 2016 The RT System itself - Status changed from 'new' to 'open'

Wed Mar 30 18:50:13 2016 cpan [...] greatships.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	Wed, 30 Mar 2016 18:49:48 -0400
To:	bug-Curses [...] rt.cpan.org
From:	LJ McDonald <cpan [...] greatships.com>

Thank you for examining this issue. After submitting this report, I found an identical bug in new_form with form fields (sounds like you are aware of it too). The exact same behaviour, workaround and fix. There are also set_menu_items and set_form_items methods that need the same fix. As for the fix itself the Perl "correct" method should be (instead of malloc in new_menu):
Show quoted text

ITEM ** item;
int count = 1;
for (item = items; *item; item++) count++;
ITEM ** new_items;
Newx(new_items, count, ITEM*);
Copy(items, new_items, count, ITEM*);
MENU * ret = new_menu(new_items);

and in free_menu (instead of free):
Show quoted text

Safefree(items);

Similarly for new_form, free_form and the methods listed above. Note "Newx", "Copy" and "Safefree" are the Perl XS memory routines/macros recommended over malloc/free.

I mentioned my Menu module I wrap these curses methods with. It works exactly as you describe the desired interface should be :) With corresponding wrappers for Fields and Forms as well. Unfortunately, the "workaround" method *is not* the correct method to use for folk who might create such objects around the Curses methods. If you use the workaround, as shown, Perl will garbage collect the variable sent in to pack once the containing block exits. And if the menu object still exists and is using the ITEMS** array, the program will (could) SEGV again. The items array is always necessary until free_menu gets called, and there is no way to guarantee that with Perl variables since Perl cannot "see" the array in use and can/will delete local variables. This is why I propose this simple fix inside the new_menu (and now new_form, set_menu_items and set_form_items). I can see no disadvantage and it's really an easy hook since menu->items (and form->items) is always available to the C portion of the library.

I was going to work up a new diff (and test) with the Perl "correct" methods shown above, but got too busy with work this week. Let me know if you would still like me to work this up.

bryanh@giraffe-data.com via RT wrote on 03/30/2016 05:25 PM:

Show quoted text

<URL: https://rt.cpan.org/Ticket/Display.html?id=113390 >

Thank you for figuring that out.  This is a pretty basic problem.

However, the solution isn't very obvious because of the unusual architecture
of the Curses extension.  It is designed to be a bare minimum interface to the
C Curses library, with the user expected to know about the C objects and
explicitly manage the memory (e.g. the user has to call free_menu, whereas in
normal Perl, just destroying all the references to the menu would be enough).

Based on that, I think the "workaround" is actually the solution that is
consistent with the rest of the interface.  The example program is wrong, and
I will fix that (and the same problem with the forms demo).

As noted under "Improvement", this is not a very good interface regardless,
and it would be really nice to have a normal Perl interface with the
underlying C library behavior hidden.  There should be no preserving of
variables, freeing, or packing.  One should create a menu with
Curses::Menu->new(@itemList) where @itemList is an array of references to item
objects.  And of course, the whole rest of the package should be the same way.

With the heavy lifting (translation to C) already done by the existing package
and extension, it should not be difficult to create a new Curses2 package in
pure Perl that has such a normal Perl interface above and uses the Curses
package below to do the work, taking care of all the hackery within.

I will look into that.

Wed Mar 30 20:12:11 2016 bryanh [...] giraffe-data.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	30 Mar 2016 23:35:58 +0000
To:	bug-Curses [...] rt.cpan.org
From:	Bryan Henderson <bryanh [...] giraffe-data.com>

I wouldn't do a wrapper the way you're thinking of. I'm talking about wrapping the entire object, not just the methods. The Curses2::Menu object would contain a reference to a Curses::Menu object. It would also contain a reference to a variable that holds that packed item list value, so it would stick around until the Curses2::Menu object is destroyed, guaranteed to be after the free_menu() has happened. -- Bryan Henderson San Jose, California

Wed Mar 30 22:38:46 2016 cpan [...] greatships.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	Wed, 30 Mar 2016 22:38:24 -0400
To:	bug-Curses [...] rt.cpan.org
From:	LJ McDonald <cpan [...] greatships.com>

bryanh@giraffe-data.com via RT wrote on 03/30/2016 08:12 PM: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=113390 > > > I wouldn't do a wrapper the way you're thinking of. I'm talking about > wrapping the entire object, not just the methods. The Curses2::Menu object > would contain a reference to a Curses::Menu object. It would also contain a > reference to a variable that holds that packed item list value, so it would > stick around until the Curses2::Menu object is destroyed, guaranteed to be > after the free_menu() has happened. >

Yes, this is the method I use. However, there is no other implementation method that would guarantee to prevent a SEGV. We should not assume a specific implementation on the part of the user to prevent the library from causing a SEGV. In my opinion, there should be no way for a user to write Perl, such as the original demo example, which would cause a SEGV. That is why I'm advocating for a fix within the library which would allow the user to write any Perl implementation he likes without such risk, and without having to document an extensive explanation in the library of why the user must allocate variables for pack, and make sure that they last as long as the menu/form. My intent of providing the workaround was to allow current users who are experiencing the SEGV a possible short-term solution until the library is fixed, especially if they are on a platform where they cannot rebuild Curses until their distribution provides a fixed one.

Wed Mar 30 23:09:59 2016 bryanh [...] giraffe-data.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	31 Mar 2016 02:46:04 +0000
To:	bug-Curses [...] rt.cpan.org
From:	Bryan Henderson <bryanh [...] giraffe-data.com>

Yes, we're both talking about the same thing: providing a module that has a normal Perl interface without any C pointers involved and the user cannot use it in any way that would cause an invalid memory reference. I'm proposing to call that module Curses2, and to leave Curses as it is for backward compatibility and also as a great tool for implementing Curses2. The existing Curses module has an entirely different goal (and I don't claim it was ever a good idea) and small changes like yours would make it a confusing hybrid of the two. The user would still be able to cause invalid memory references many ways, for example by doing that pack incorrectly. -- Bryan Henderson San Jose, California

Thu Mar 31 02:20:45 2016 cpan [...] greatships.com - Correspondence added

Subject:	Re: [rt.cpan.org #113390] SEGV in Curses (menus)
Date:	Thu, 31 Mar 2016 02:20:27 -0400
To:	bug-Curses [...] rt.cpan.org
From:	LJ McDonald <cpan [...] greatships.com>

bryanh@giraffe-data.com via RT wrote on 03/30/2016 11:10 PM: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=113390 > > > Yes, we're both talking about the same thing: providing a module that has > a normal Perl interface without any C pointers involved and the user cannot > use it in any way that would cause an invalid memory reference. > > I'm proposing to call that module Curses2, and to leave Curses as it is for > backward compatibility and also as a great tool for implementing Curses2.

Actually, we're not talking the same thing. I'm not proposing a "Curses2" or any such substantial modification. I did offer up an "improvement" in my original report, but I understood your original response as to why that is not appropriate and/or desired. My point was that if the current Curses module is left unpatched, the user has to go to extraordinary and undocumented measures, far beyond the messy "pack" construct (which is adequately documented), and be exceedingly careful to make sure his Perl "pack" variables have the correct scope and lifetime in order to even *use* the current Curses module as provided without risk of SEGV. The original demo program will not run, as is, and I'm sure there is other code written by users based on those examples which is at risk of SEGV. In order for the Curses module to function *as originally advertised* it must be patched to fix this SEGV. I know it is scary to even touch such a long-standing module. But when the module is found to have a fundamental flaw such as this, it must be fixed. The correct procedure would be to create a new version number with the relevant patch applied. This addresses any compatibility concerns - users can choose to upgrade, or not. But at least a user caught in the "trap" I found myself has an option to fix mysterious SEGVs in existing code, without being required to rewrite that code with some obscure workaround that requires precise understanding of the subtleties involved in order to even function. I again offer such a patch for your review. This time with fixes for both the items problem and the form fields problem, written in "correct" Perl XS syntax, with understandable comments. In my opinion, this minimal patch fixes this problem and maintains 100% compatibility with existing programs. I believe the patch to be consistent with the original objectives of the Curses module, and simply restores it to the "as written" functionality that was originally thought to be there.

Message body is not shown because sender requested not to inline it.