Bug#811640: regarding dasher gcc6 ftbfs / 5.0 beta
Hi Andreas! Andreas Henriksson: > Read your bug report (#835533) and spent a few seconds looking at > the gcc6 ftbfs (#811640). Thanks a lot! > The current ftbfs looks trivial to resolve bit apparently noone > cares enough about the package. OK, this is a useful data point. > Since you mentioned there was upstream activity I looked at how they solved > it in: > https://github.com/ipomoena/dasher/blob/master/Src/Gtk2/DasherAppSettings.cpp#L81 > (Not sure this is the correct upstream repository though.) > This does not look correct at all to me. > [...] > Without investigating any deeper I think this must result in a use-after-free. > We need someone that's interested in dasher to maintain it properly. > Apparently upstream could use an extra pair of eyes to help keep them > safe. Good to know. > This is thus my attempt at suckering you, who have shown atleast > a bit of interest in dasher, to be that person. I'm afraid I cannot be that person, simply because I am mostly illiterate at C/C++. Sorry! But I do appreciate your attempt at fixing the social/organizational problem that's hidden behind the mere surface of FTBFS bug reports :) > If not, the quick route would probably just be to add a patch with > the above suggested solution (which I've verified fixes the build) > and do an NMU to get dasher back into testing. I'll think about it. In theory I could do that, but I'm not keen on "forcing" into a Debian stable release software that 1. will soon be obsolete since upstream has a new major version in the works; 2. gets little care from its formal maintainers. If I did that I would feel responsible to effectively maintain the package in stable (which can imply getting fixes in sid first) during the lifetime of Stretch, which would not be reasonable a commitment to make, even though there are great chances that the maintenance work takes absolutely no time. Should I point the Debian Accessibility team / ML to this bug? Cheers, -- intrigeri
Bug#811640: regarding dasher gcc6 ftbfs / 5.0 beta
Hello intrigeri!
Read your bug report (#835533) and spent a few seconds looking at
the gcc6 ftbfs (#811640). The current ftbfs looks trivial to resolve
bit apparently noone cares enough about the package.
> DasherAppSettings.cpp:398:14: error: cannot convert 'bool' to 'const gchar*
> {aka const char*}' in return
>return false;
> ^
Looking at the callers of the function the correct return should apparently
be "" here as callers checks for that or for strlen == 0.
> DasherAppSettings.cpp: In function 'void
> dasher_app_settings_launch_advanced(DasherAppSettings*)':
> DasherAppSettings.cpp:505:15: warning: ISO C++ forbids converting a string
> constant to 'gchar* {aka char*}' [-Wwrite-strings]
>szArgs[0] = "gconf-editor";
>^~
This one is not so obvious to me. For "C++ correctness" you should apparently
change the declaration of szArgs to be const char* rather than char*, but
I think that'll not last long as szArgs is passed to g_spawn_async
which doesn't take a 'const' char*, only a char*. I'm not sure if there's
any reason g_spawn_async hasn't been constified, but I assume it mostly
that it was written with C in mind and this is no problem there...
My spontaneous idea for solution was thus to simply cast the assignments:
szArgs[0] = (char*)"gconf-editor";
szArgs[1] = (char*)"/apps/dasher4";
Since you mentioned there was upstream activity I looked at how they solved
it in:
https://github.com/ipomoena/dasher/blob/master/Src/Gtk2/DasherAppSettings.cpp#L81
(Not sure this is the correct upstream repository though.)
This does not look correct at all to me. Please note the strdup which will
need it to get freed at some point to not leak memory, then call to
g_spawn_async and given it's an async method I assume it won't really
do any work at all until we let the main loop run again, then *directly*
calls g_strfreev(szArgs);
Without investigating any deeper I think this must result in a use-after-free.
We need someone that's interested in dasher to maintain it properly.
Apparently upstream could use an extra pair of eyes to help keep them
safe. This is thus my attempt at suckering you, who have shown atleast
a bit of interest in dasher, to be that person.
If not, the quick route would probably just be to add a patch with
the above suggested solution (which I've verified fixes the build)
and do an NMU to get dasher back into testing.
Regards,
Andreas Henriksson
