[XForms] Re: Xforms : popup menus

[Jean-Marc Lasgouttes]

>>>>> "laurent" == laurent FOURNIER <laurent.fournier at lapp.in2p3.fr> writes:

laurent> Jean-Marc Lasgouttes wrote:
laurent> I agree with your point of view. The use of memset() is only
laurent> a way to ensure data initialization. As a rule of thumb we
laurent> develop our code assuming that a NULL pointer is free, then
laurent> we use to call calloc(), or realloc() with a memset() on any
laurent> additional data. The best way would be to use the exact
laurent> amount of memory we need for the menu item array.

Sure. I memory very tight in your setting?

laurent> The data what causes problems is the menu item array, when
laurent> setting fl_maxpup to a higher value than FL_MAXPUP with
laurent> fl_setpup_maxpup(), i.e. the pointer to menu items and the
laurent> menu items themselves are not NULL. 

There is not real pointer to menu items, since item is an array. All
the code that I have seen sets the first entry (item[0]) to 0 when
needed, so there should not be any problem [actually, I know there is
a problem, since you experienced it. But I fail to see it]

laurent> When calling fl_setpup_maxpup(), the default array of popup
laurent> menus is resized and then reallocated. But at the
laurent> initialization time (the very first call to fl_init_pup), the
laurent> first allocation is made using a calloc() which sets all data
laurent> to zero, and in particular the pointer "menu_rec->item". When
laurent> not set to zero, we get garbage into the pointer array which
laurent> leads to invalid data segments on the array (and thus menu
laurent> items), and LynxOS gives a SIGSEGV when freeing any segment
laurent> beginning with '9' (these are reserved adresses for the
laurent> kernel but this is not the only case with this very sensitive
laurent> OS).

fl_setpup_maxpup does reset title and item[0] for each new popup. It
seems however that it fails to reset parent, and this may cause
problems with the following test in find_index:
	if (!p->title && !p->item[0] && !p->parent)


Then fl_setpup_maxpup would read:

int
fl_setpup_maxpup(int n)
{
    int i;

    if (n < FL_MAXPUP)
	return FL_MAXPUP;

    fl_init_pup();

    menu_rec = fl_realloc(menu_rec, n * sizeof(*menu_rec));
    for (i = fl_maxpup; i < n; i++)
    {
	menu_rec[i].title = 0;
	menu_rec[i].item[0] = 0;
	menu_rec[i].parent = 0; /* <================== */
    }

    return fl_maxpup = n;
}

However, I fail to see how this missing piece could cause a crash. As
far as I can see, the only possible consequence is that find_index
would fail to recognize a popup as available. 

The fix is probably good nevertheless.


laurent> If you prefer not to call memset(), one solution to the real
laurent> problem could be : 



laurent> 1 - to ensure proper initialization of unused pointers
laurent> (especially menu_rec-> item of course) when calling
laurent> fl_setpup_maxpup(), 

But menu_rec->item is not really a pointer, is it?

laurent> 2 - to limit the loop counter in the function fl_freepup() to
laurent> p->nitem instead of FL_MAXPUPI (line 1157 in
laurent> v0.9999/lib/xpopup.c) because p->nitem handles the number of
laurent> actually allocated menu item p->structures
laurent> (i.e. valid pointers).

Yes, this one seems to be the real killer. I'll try to reproduce its
effect under valgrind here.

laurent> Anyhow, fl_safe_free() checks only NULL pointers and is not
laurent> enough for invalid pointers unless you ensure that a NULL
laurent> pointer is free. This is the main reason for crashes under
laurent> LynxOS with the so-called '9' segment. Moreover, memset()
laurent> gives a slightly shorter code because we do not have to set
laurent> many fields to 0 (we have limited space on Lynx CPUs).

I doubt that this difference in space will be important enough. I
guess there many other sources of bloat in xforms :)

laurent> This is quite true. My application works fine when allocating
laurent> a (big) default number of popups (and a call to memset() at
laurent> this time). The only thing what I have to assume is to ensure
laurent> enough (initialized) memory space. Moreover, reallocating
laurent> memory for popups during the running phase seems not to be a
laurent> problem. But in that case I need to gain access to the static
laurent> variable fl_max_pup to count the number of popups and get
laurent> more space when needed.

What we do in LyX is that we maintain our own maxpup variable that
starts at 32 and we call fl_setpup_maxpup as needed. The (C++) code
looks like

int get_new_submenu(vector<int> & smn, Window win)
{
	static size_type max_number_of_menus = 32;
	if (smn.size() >= max_number_of_menus)
		max_number_of_menus =
		    fl_setpup_maxpup(static_cast<int>(2*smn.size()));
[...]

Here, smn holds the list of menus that have been allocated. This is
used to deallocate them when they ar enot needed any more (menus are
generated just before displaying them in LyX).


I'll try to come up with a patch against xforms 1.0.90. I'd be
interested to see whether it fixes your problems.

JMarc