Fl_Menu_::copy() just evil, what to do?

[Pierre Ossman]

Okay, so I've run into a rather nasty bug in the API and I'm not sure
how to proceed.

It started with the fact that Fl_Menu_::add() and friends like to
assign special meaning to "/". And since you cannot escape things I had
to go looking for alternatives. Fl_Menu_::menu() seemed like a nice
choice and it seemed to do what I wanted. But it required the given
array to live on, so it didn't fit in all scenarios. Fortunately there
is also Fl_Menu_::copy() that is documented as making its own copy.

Not quite though.

It does quite happily make a copy of the array of Fl_Menu_Item:s. But
it completely forgets about all the embedded strings. However
Fl_Menu_::clear() still thinks it owns these strings so it calls free()
on them.

So this is apparently the safe and correct way to use the API:

Fl_Menu_Item popup[] = {
{strdup("&alpha"), FL_ALT+'a', the_cb, (void*)1},
{strdup("&beta"), FL_ALT+'b', the_cb, (void*)2},
{strdup("gamma"), FL_ALT+'c', the_cb, (void*)3, FL_MENU_DIVIDER},
{0}};
menu->copy(popup);

Beautiful.

We can't really fix the API because that's an API change that will leak
memory for those using the API "correctly" today. So I'm not sure what
to do. I'm thinking something along the lines of:

- Document Fl_Menu_::menu() as inherently unsafe as you have to
provide different data to it depending on if Fl_Menu_::add() and
friends will be called or not.

- Document the idiotic strdup() requirement for Fl_Menu_::copy().

- Introduce a Fl_Menu_::copy2() with sane behaviour.

Rgds
--
Pierre Ossman Software Development
Cendio AB https://cendio.com
Teknikringen 8 https://twitter.com/ThinLinc
583 30 Linköping https://facebook.com/ThinLinc
Phone: +46-13-214600 https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[Greg Ercolano]

On 02/20/15 01:01, Pierre Ossman wrote:
> It started with the fact that Fl_Menu_::add() and friends like to
> assign special meaning to "/". And since you cannot escape things I had
> to go looking for alternatives.

Wait, lost me there -- you can escape / with \, e.g.

#include <FL/Fl.H>
#include <FL/Fl_Window.H>
#include <FL/Fl_Menu_Bar.H>
#include <stdlib.h>
int main() {
Fl_Window *win = new Fl_Window(400,400);
Fl_Menu_Bar *menu = new Fl_Menu_Bar(0,0,400,25);
menu->add("File/Quit");
menu->add("Directories/\\/var\\/tmp");
menu->add("Directories/\\/usr\\/tmp");
menu->add("Directories/\\/lib\\/X11");
win->end();
win->show();
return(Fl::run());
}

Of course double backslashes are needed for FLTK to see a single
backslash, to get past the C++ interpreter's string parsing of "\"
for "\n" et al.

When I run the above, I see a "Directories" menu with e.g.
"/var/tmp" in it (and not a "var" submenu containing "tmp")

Can't answer to the rest of the msg; kicking off for the day.

[Pierre Ossman]

On Fri, 20 Feb 2015 01:53:04 -0800,
Greg Ercolano wrote:

> On 02/20/15 01:01, Pierre Ossman wrote:
> > It started with the fact that Fl_Menu_::add() and friends like to
> > assign special meaning to "/". And since you cannot escape things I had
> > to go looking for alternatives.
>
> Wait, lost me there -- you can escape / with \, e.g.
>

Huh. My colleague claimed that didn't work. There's also this in the
API docs for Fl_Menu_::replace():

"This is the only way to get slash into an add()'ed menu item. If the
menu array was directly set with menu(x) then copy() is done to make a private array."

Still, these strings tend to be translated so you'd have to automate
the escaping somehow. A bit of a pain. Being able to give names without
that magic is preferred.

[Albrecht Schlosser]

On 20.02.2015 10:02 Pierre Ossman wrote:
> Okay, so I've run into a rather nasty bug in the API and I'm not sure
> how to proceed.
>

> It started with the fact that Fl_Menu_::add() and friends like to
> assign special meaning to "/". And since you cannot escape things I had

> to go looking for alternatives. Fl_Menu_::menu() seemed like a nice
> choice and it seemed to do what I wanted. But it required the given
> array to live on, so it didn't fit in all scenarios. Fortunately there
> is also Fl_Menu_::copy() that is documented as making its own copy.
>
> Not quite though.
>
> It does quite happily make a copy of the array of Fl_Menu_Item:s. But
> it completely forgets about all the embedded strings. However
> Fl_Menu_::clear() still thinks it owns these strings so it calls free()
> on them.

Hmm, are you sure? You should double check this. I looked into the code
today, but it's too confusing to grok it in a short time. However I saw
that there is a member Fl_Menu_::alloc that can be 0, 1, or 2 (despite
the comment that says only 0 or 1). Only if it is 2 are the strings
free'd. In Fl_Menu_add.cxx, line #384 it is set to 2 in
Fl_Menu_::insert() under certain conditions only. I'm not sure that all
is okay or as documented or expected, but I don't know if your concerns
are correct (but I don't dare saying they are not!).

> So this is apparently the safe and correct way to use the API:
>
> Fl_Menu_Item popup[] = {
> {strdup("&alpha"), FL_ALT+'a', the_cb, (void*)1},
> {strdup("&beta"), FL_ALT+'b', the_cb, (void*)2},
> {strdup("gamma"), FL_ALT+'c', the_cb, (void*)3, FL_MENU_DIVIDER},
> {0}};
> menu->copy(popup);
>
> Beautiful.

I'd check that again. Why copy? Why strdup? The menu is designed to be
left alone if given by the user, but copied internally when necessary
(i.e. when calling add()). I'd be surprised if this was so buggy, but
again: I'm really not sure, and I didn't write nor comprehend the menu
code fully.

> We can't really fix the API because that's an API change that will leak
> memory for those using the API "correctly" today. So I'm not sure what
> to do. I'm thinking something along the lines of:
>
> - Document Fl_Menu_::menu() as inherently unsafe as you have to
> provide different data to it depending on if Fl_Menu_::add() and
> friends will be called or not.

That should not be the case.

> - Document the idiotic strdup() requirement for Fl_Menu_::copy().
>
> - Introduce a Fl_Menu_::copy2() with sane behaviour.

Something like that might be useful anyway. Maybe a way to say: it's all
allocated data, use it and free it yourself (give ownership of strings
(labels) to FLTK as well, like copy_label()), i.e. force setting alloc
to 2 internally, or something like this.

[Pierre Ossman]

On Sat, 21 Feb 2015 01:13:27 +0100,
Albrecht Schlosser wrote:

>
> Hmm, are you sure? You should double check this. I looked into the code
> today, but it's too confusing to grok it in a short time. However I saw
> that there is a member Fl_Menu_::alloc that can be 0, 1, or 2 (despite
> the comment that says only 0 or 1). Only if it is 2 are the strings
> free'd. In Fl_Menu_add.cxx, line #384 it is set to 2 in
> Fl_Menu_::insert() under certain conditions only. I'm not sure that all
> is okay or as documented or expected, but I don't know if your concerns
> are correct (but I don't dare saying they are not!).
>

Urgh. You are right. That however just makes things even more
confusing.

Look at how insert() works. If the menu started off using menu() or
copy() then it will keep alloc at 1. But the strings given to insert()
are still strdup():ed. IOW they will leak.

And replace() is even more fun. It will strdup() or not depending on
how the current menu was created. Which the caller might not have any
idea about.

> > So this is apparently the safe and correct way to use the API:
> >
> > Fl_Menu_Item popup[] = {
> > {strdup("&alpha"), FL_ALT+'a', the_cb, (void*)1},
> > {strdup("&beta"), FL_ALT+'b', the_cb, (void*)2},
> > {strdup("gamma"), FL_ALT+'c', the_cb, (void*)3, FL_MENU_DIVIDER},
> > {0}};
> > menu->copy(popup);
> >
> > Beautiful.
>
> I'd check that again. Why copy? Why strdup? The menu is designed to be
> left alone if given by the user, but copied internally when necessary
> (i.e. when calling add()). I'd be surprised if this was so buggy, but
> again: I'm really not sure, and I didn't write nor comprehend the menu
> code fully.
>

copy() is most definitely needed since popup here lives on the stack
and menu might live longer than that. The strings didn't need to be
duplicated here though as I didn't see the full extent of "alloc". If
they were also on the stack they would have to be though, even if that
means they would leak.

> >
> > - Introduce a Fl_Menu_::copy2() with sane behaviour.
>
> Something like that might be useful anyway. Maybe a way to say: it's all
> allocated data, use it and free it yourself (give ownership of strings
> (labels) to FLTK as well, like copy_label()), i.e. force setting alloc
> to 2 internally, or something like this.
>

I think that's really the only sane way for an API like this. It
doesn't really solve my current issue though...

[Albrecht Schlosser]

On 23.02.2015 10:04 Pierre Ossman wrote:
> On Sat, 21 Feb 2015 01:13:27 +0100,
> Albrecht Schlosser wrote:
>

>> Hmm, are you sure? You should double check this. I looked into the code
>> today, but it's too confusing to grok it in a short time. However I saw
>> that there is a member Fl_Menu_::alloc that can be 0, 1, or 2 (despite
>> the comment that says only 0 or 1). Only if it is 2 are the strings
>> free'd. In Fl_Menu_add.cxx, line #384 it is set to 2 in
>> Fl_Menu_::insert() under certain conditions only. I'm not sure that all
>> is okay or as documented or expected, but I don't know if your concerns
>> are correct (but I don't dare saying they are not!).
>

> Urgh. You are right. That however just makes things even more
> confusing.

Yep. It's really confusing.

> Look at how insert() works. If the menu started off using menu() or
> copy() then it will keep alloc at 1. But the strings given to insert()
> are still strdup():ed. IOW they will leak.

I got that impression as well, but could not confirm it in the time I
spent to look at the code. I went in circles.

> And replace() is even more fun. It will strdup() or not depending on
> how the current menu was created. Which the caller might not have any
> idea about.

We can perhaps fix this by strdup()'ing everything once we have to copy
at least one menu text and then setting alloc to 2. This doesn't prevent
the initial strings from leaking, but makes all additional strings work
correctly (being copied and free'd). I wouldn't care much about the
initial strings "leaking" because they will most probably be strings in
static memory. In other cases we'd need the new API feature.

That means: if you construct a menu with static strings, don't care. If
you construct menu strings elsewhere (in local variables or malloc'd),
use the new API to make sure they are copied. In this case you can free
the memory or let the local variable go out of scope as soon as the menu
is created or copied etc.

In all other cases (legacy code) there can only be leaks of the initial
strings if they are alloc'd and not in static memory, which would be
negligible.

>>> So this is apparently the safe and correct way to use the API:
>>>
>>> Fl_Menu_Item popup[] = {
>>> {strdup("&alpha"), FL_ALT+'a', the_cb, (void*)1},
>>> {strdup("&beta"), FL_ALT+'b', the_cb, (void*)2},
>>> {strdup("gamma"), FL_ALT+'c', the_cb, (void*)3, FL_MENU_DIVIDER},
>>> {0}};
>>> menu->copy(popup);
>>>
>>> Beautiful.
>>
>> I'd check that again. Why copy? Why strdup? The menu is designed to be
>> left alone if given by the user, but copied internally when necessary
>> (i.e. when calling add()). I'd be surprised if this was so buggy, but
>> again: I'm really not sure, and I didn't write nor comprehend the menu
>> code fully.
>>
>

> copy() is most definitely needed since popup here lives on the stack
> and menu might live longer than that.

True.

But you didn't say that this was in a subroutine or function ;-)

> The strings didn't need to be
> duplicated here though as I didn't see the full extent of "alloc". If
> they were also on the stack they would have to be though, even if that
> means they would leak.

... and here comes the new method or option/argument into play. We need
something like Fl_Widget::copy_label() to make sure we can free strings
we constructed elsewhere (not in static memory) right after giving them
to the menu and we must make sure that static strings are not free'd.

>>> - Introduce a Fl_Menu_::copy2() with sane behaviour.
>>
>> Something like that might be useful anyway. Maybe a way to say: it's all
>> allocated data, use it and free it yourself (give ownership of strings
>> (labels) to FLTK as well, like copy_label()), i.e. force setting alloc
>> to 2 internally, or something like this.
>>
>

> I think that's really the only sane way for an API like this.

Maybe we can add another flag bit to each menu entry saying that _this_
menu string is copied and needs to be free'd instead of having only one
global flag. If that helps. So you could also malloc the string and set
the "allocated" bit for this string and let the menu code free it when
not needed any more.

After thinking more about it, I like this idea. One bit in the menu
array per string instead of one global member alloc == 2.

> It doesn't really solve my current issue though...

... and your current issue is? You started your description with
Fl_Menu_::add() and the problem of escaping '/', but I don't know
exactly what the underlying problem is. Could you elaborate?

[Pierre Ossman]

On Mon, 23 Feb 2015 11:15:37 +0100,
Albrecht Schlosser wrote:

> > It doesn't really solve my current issue though...
>
> ... and your current issue is? You started your description with
> Fl_Menu_::add() and the problem of escaping '/', but I don't know
> exactly what the underlying problem is. Could you elaborate?
>

I have a bunch of menu entries where the text is not fully trustworthy.
Mostly it's translations, but it could be other things. I'd like to be
able to add those without it doing magical things with slashes. Doing
on the fly escaping is possible but does make things a bit messy.

[Albrecht Schlosser]

On 23.02.2015 12:13 Pierre Ossman wrote:
> On Mon, 23 Feb 2015 11:15:37 +0100,
> Albrecht Schlosser wrote:
>

>>> It doesn't really solve my current issue though...
>>
>> ... and your current issue is? You started your description with
>> Fl_Menu_::add() and the problem of escaping '/', but I don't know
>> exactly what the underlying problem is. Could you elaborate?
>>
>

> I have a bunch of menu entries where the text is not fully trustworthy.
> Mostly it's translations, but it could be other things. I'd like to be
> able to add those without it doing magical things with slashes. Doing
> on the fly escaping is possible but does make things a bit messy.

I see. So we need a way to handle strdup()'ing correctly, and we should
have an option to do '/' processing or prohibit it when adding menu
entries with add() and maybe other methods. In fact there should
probably be an option to suppress all special character processing.

The docs say: "The label string is copied to new memory and can be
freed." According to your observations (I'm not sure) this could mean
that the copied string will not be freed by FLTK when the menu is
deleted, if the internal alloc flag is 0 or 1 in this menu. This would
be a bug, IMHO. Is this correct?

[Pierre Ossman]

On Mon, 23 Feb 2015 20:48:13 +0100,
Albrecht Schlosser wrote:

> On 23.02.2015 12:13 Pierre Ossman wrote:
> > On Mon, 23 Feb 2015 11:15:37 +0100,
> > Albrecht Schlosser wrote:
> >
> >>> It doesn't really solve my current issue though...
> >>
> >> ... and your current issue is? You started your description with
> >> Fl_Menu_::add() and the problem of escaping '/', but I don't know
> >> exactly what the underlying problem is. Could you elaborate?
> >>
> >
> > I have a bunch of menu entries where the text is not fully trustworthy.
> > Mostly it's translations, but it could be other things. I'd like to be
> > able to add those without it doing magical things with slashes. Doing
> > on the fly escaping is possible but does make things a bit messy.
>
> I see. So we need a way to handle strdup()'ing correctly, and we should
> have an option to do '/' processing or prohibit it when adding menu
> entries with add() and maybe other methods. In fact there should
> probably be an option to suppress all special character processing.
>

In my case I'd probably like the & processing to stay in some cases. So
some flexibility would be nice. :)

> The docs say: "The label string is copied to new memory and can be
> freed." According to your observations (I'm not sure) this could mean
> that the copied string will not be freed by FLTK when the menu is
> deleted, if the internal alloc flag is 0 or 1 in this menu. This would
> be a bug, IMHO. Is this correct?
>

That's how I read the code, yes. I have not verified it properly though.

[Albrecht Schlosser]

On 24.02.2015 16:42 Pierre Ossman wrote:
> On Mon, 23 Feb 2015 20:48:13 +0100,
> Albrecht Schlosser wrote:
>

>> On 23.02.2015 12:13 Pierre Ossman wrote:
>>> I have a bunch of menu entries where the text is not fully trustworthy.
>>> Mostly it's translations, but it could be other things. I'd like to be
>>> able to add those without it doing magical things with slashes. Doing
>>> on the fly escaping is possible but does make things a bit messy.
>>
>> I see. So we need a way to handle strdup()'ing correctly, and we should
>> have an option to do '/' processing or prohibit it when adding menu
>> entries with add() and maybe other methods. In fact there should
>> probably be an option to suppress all special character processing.
>

> In my case I'd probably like the & processing to stay in some cases. So
> some flexibility would be nice. :)

What about an optional argument 'const char *special' with the following
interpretation:

- NULL pointer (the default): everything allowed (as it is today)
- "" (empty string): nothing allowed
- other string: allowed special characters

"Allowed" means here: will be processed in its special meaning,
otherwise it will be used as a normal character. "Everything" means the
current or maybe (later) extended set of special characters.

Or we can use a bitmask for allowed special characters with the default
value = 0xff..ff (all bits set), but then we need an enum with all
special characters that can be or'ed together.

>> The docs say: "The label string is copied to new memory and can be
>> freed." According to your observations (I'm not sure) this could mean
>> that the copied string will not be freed by FLTK when the menu is
>> deleted, if the internal alloc flag is 0 or 1 in this menu. This would
>> be a bug, IMHO. Is this correct?
>

> That's how I read the code, yes. I have not verified it properly though.

Same here, but from what I read I suspect that it is something like that.

[ercola...@gmail.com]

Old as this thread is, I'm opening an issue for it: issue #332