Hi,
What is the reason for configuration_save() and configuration_save_default_session() to use g_strconcat() instead of utils_build_path()? Some internal function calls utils_build_path(), or?...
On 10 April 2011 03:48, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
Hi,
What is the reason for configuration_save() and configuration_save_default_session() to use g_strconcat() instead of utils_build_path()? Some internal function calls utils_build_path(), or?...
The problem is that utils_build_path is *waaaaay* unsafe, it re-uses a single buffer so it can't be called again before the caller has finished with the result, unless the caller copies the result.
It should only be used where the returned string is finished with or copied before any calls to other functions. It should never be used anywhere that a caller or callee might use it.
utils_build_path is really a bomb waiting to explode in someone's face (well code anyway :-). It shouldn't be a general util, either it should be made safe or if some part of Geany uses enough paths that it really is a material overhead to allocate and deallocate buffers then it should be made a local function there, not a general util.
The configuration_save* functions are called from lots of places, if any of them have used utils_build_path then configuration_save* can't, so best they don't.
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Sun, 10 Apr 2011 10:22:36 +1000 Lex Trotman elextr@gmail.com wrote:
On 10 April 2011 03:48, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
What is the reason for configuration_save() and configuration_save_default_session() to use g_strconcat() instead of utils_build_path()? [...]
The problem is that utils_build_path is *waaaaay* unsafe, it re-uses a single buffer so it can't be called again before the caller has finished with the result, unless the caller copies the result.
I know, my question was is there a known conflict...
[...]
utils_build_path is really a bomb waiting to explode in someone's face (well code anyway :-). It shouldn't be a general util, either it should be made safe or if some part of Geany uses enough paths that it really is a material overhead to allocate and deallocate buffers then it should be made a local function there, not a general util.
+1. On a second thought, +10.
The configuration_save* functions are called from lots of places, if any of them have used utils_build_path then configuration_save* can't, so best they don't.
Only 2 places for configuration_save() (3 with x11 session management), and 1 for configuration_save_default_session(), and all seem safe. Comparing with the load functions, perhaps the reason is that saves use configfile twice, with a lot of function calls between.
On 10 April 2011 20:16, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Sun, 10 Apr 2011 10:22:36 +1000 Lex Trotman elextr@gmail.com wrote:
On 10 April 2011 03:48, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
What is the reason for configuration_save() and configuration_save_default_session() to use g_strconcat() instead of utils_build_path()? [...]
The problem is that utils_build_path is *waaaaay* unsafe, it re-uses a single buffer so it can't be called again before the caller has finished with the result, unless the caller copies the result.
I know, my question was is there a known conflict...
Yeah well, even if its ok today, whats the chances someone forgets and makes a change tomorrow ... like me :-)
[...]
utils_build_path is really a bomb waiting to explode in someone's face (well code anyway :-). It shouldn't be a general util, either it should be made safe or if some part of Geany uses enough paths that it really is a material overhead to allocate and deallocate buffers then it should be made a local function there, not a general util.
+1. On a second thought, +10.
In fact its such a small function that even if a couple of places need it, having a few local copies isn't going to add materially to code size.
The configuration_save* functions are called from lots of places, if any of them have used utils_build_path then configuration_save* can't, so best they don't.
Only 2 places for configuration_save() (3 with x11 session management), and 1 for configuration_save_default_session(), and all seem safe. Comparing with the load functions, perhaps the reason is that saves use configfile twice, with a lot of function calls between.
Kinda messy, the configuration_save() calls are in prefs.c and callbacks.c and configuration_save_default_session() is in project.c so they are all over the place, I think it makes it very brittle.
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On 10 April 2011 20:53, Lex Trotman elextr@gmail.com wrote:
On 10 April 2011 20:16, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Sun, 10 Apr 2011 10:22:36 +1000 Lex Trotman elextr@gmail.com wrote:
On 10 April 2011 03:48, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
What is the reason for configuration_save() and configuration_save_default_session() to use g_strconcat() instead of utils_build_path()? [...]
The problem is that utils_build_path is *waaaaay* unsafe, it re-uses a single buffer so it can't be called again before the caller has finished with the result, unless the caller copies the result.
I know, my question was is there a known conflict...
Yeah well, even if its ok today, whats the chances someone forgets and makes a change tomorrow ... like me :-)
On thinking about it some more, if the g_strconcat worries you, do as the comment on util_build_path says, use g_build_path :-)
Cheers Lex
On Sun, 10 Apr 2011 13:16:26 +0300 Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
utils_build_path is really a bomb waiting to explode in someone's face (well code anyway :-). It shouldn't be a general util, either it should be made safe or if some part of Geany uses enough paths that it really is a material overhead to allocate and deallocate buffers then it should be made a local function there, not a general util.
+1. On a second thought, +10.
The reason I wrote it wasn't efficiency, but to avoid having to manually free the string each time. I miss RAII :-/
I agree it's not safe and maybe we should remove it. I deliberately didn't add it to the plugin API for this reason.
Regards, Nick