Just after configuring the location, at the first data fetch, the plugin crashes. stderrs contains : (xfce4-weather-plugin:17473): weather-WARNING **: getting http://api.yr.no//weatherapi/locationforecastlts/1.1/?lat=45.7059321003807;lon=3.01586471282173 xfce4-weather-plugin in free(): error: chunk is already free 0x7e7a77c0 #3 0x0b148d29 in free (ptr=0x7d8a9020) at /usr/src/lib/libc/stdlib/malloc.c:1243 #4 0x03ab7322 in g_free () from /usr/local/lib/libglib-2.0.so.3200.0 #5 0x1c00e82e in parse_location (cur_node=0x7e7d2f40, loc=0x892cf318) at weather-parsers.c:170 #6 0x1c00eed5 in parse_time (cur_node=0x7e7c9640, data=0x7fabb800) at weather-parsers.c:137 #7 0x1c00f02d in parse_weather (cur_node=0x7e7b7b00) at weather-parsers.c:80 #8 0x1c0085fe in cb_update (succeed=1, result=0x7e74d000 "<weatherdata xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:noNamespaceSchemaLocation=\"http://api.met.no/weatherapi/locationforecastlts/1.1/schema\" created=\"2012-07-23T16:04:36Z\">\n <meta>"..., len=53853, user_data=0x7ccba860) at weather.c:371 #9 0x1c00db6d in weather_http_receive_data_destroyed (user_data=0x7ffbd180) at weather-http.c:450 #10 0x03aaf3c4 in g_source_set_callback () from /usr/local/lib/libglib-2.0.so.3200.0 #11 0x03aaf7f2 in g_source_unref () from /usr/local/lib/libglib-2.0.so.3200.0 #12 0x03aafaa4 in g_main_context_dispatch () from /usr/local/lib/libglib-2.0.so.3200.0 #13 0x03ab22c9 in g_main_context_prepare () from /usr/local/lib/libglib-2.0.so.3200.0 #14 0x03ab248c in g_main_loop_run () from /usr/local/lib/libglib-2.0.so.3200.0 #15 0x0bebef44 in gtk_main () from /usr/local/lib/libgtk-x11-2.0.so.2400.0 #16 0x1c0075de in main (argc=8, argv=0xcfbf2fec) at weather.c:1189 Maybe weather-parsers.c could check if the loc members were not already free'ed ? usually after free you set the ptr to NULL and check against that before retrying to free it. On another note, the plugin still installs in libexec/xfce4, while now all other plugins installs to lib/xfce4. I can send you the patch to fix that and build the plugin as a module if you want.
Note that when tracing with gdb, the crash also happens in parse_time: #3 0x0b2cdd29 in free (ptr=0x888b92c0) at /usr/src/lib/libc/stdlib/malloc.c:1243 #4 0x1c00ee8d in parse_time (cur_node=0x7c9b31c0, data=0x88d25000) at weather-parsers.c:120 #5 0x1c00f02d in parse_weather (cur_node=0x7c9b77c0) at weather-parsers.c:80 So there might be too many free's() around.. though that one puzzles me.
(In reply to comment #0) > Just after configuring the location, at the first data fetch, the plugin > crashes. > stderrs contains : > (xfce4-weather-plugin:17473): weather-WARNING **: getting > http://api.yr.no//weatherapi/locationforecastlts/1.1/?lat=45.7059321003807; > lon=3.01586471282173 > xfce4-weather-plugin in free(): error: chunk is already free 0x7e7a77c0 > > #3 0x0b148d29 in free (ptr=0x7d8a9020) at > /usr/src/lib/libc/stdlib/malloc.c:1243 > #4 0x03ab7322 in g_free () from /usr/local/lib/libglib-2.0.so.3200.0 > #5 0x1c00e82e in parse_location (cur_node=0x7e7d2f40, loc=0x892cf318) at > weather-parsers.c:170 > #6 0x1c00eed5 in parse_time (cur_node=0x7e7c9640, data=0x7fabb800) at > weather-parsers.c:137 > #7 0x1c00f02d in parse_weather (cur_node=0x7e7b7b00) at weather-parsers.c:80 > #8 0x1c0085fe in cb_update (succeed=1, > result=0x7e74d000 "<weatherdata > xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" > xsi:noNamespaceSchemaLocation=\"http://api.met.no/weatherapi/ > locationforecastlts/1.1/schema\" created=\"2012-07-23T16:04:36Z\">\n > <meta>"..., len=53853, user_data=0x7ccba860) > at weather.c:371 > #9 0x1c00db6d in weather_http_receive_data_destroyed (user_data=0x7ffbd180) > at weather-http.c:450 > #10 0x03aaf3c4 in g_source_set_callback () from > /usr/local/lib/libglib-2.0.so.3200.0 > #11 0x03aaf7f2 in g_source_unref () from /usr/local/lib/libglib-2.0.so.3200.0 > #12 0x03aafaa4 in g_main_context_dispatch () from > /usr/local/lib/libglib-2.0.so.3200.0 > #13 0x03ab22c9 in g_main_context_prepare () from > /usr/local/lib/libglib-2.0.so.3200.0 > #14 0x03ab248c in g_main_loop_run () from > /usr/local/lib/libglib-2.0.so.3200.0 > #15 0x0bebef44 in gtk_main () from /usr/local/lib/libgtk-x11-2.0.so.2400.0 > #16 0x1c0075de in main (argc=8, argv=0xcfbf2fec) at weather.c:1189 > > Maybe weather-parsers.c could check if the loc members were not already > free'ed ? usually after free you set the ptr to NULL and check against that > before retrying to free it. It doesn't do that for the rest of the members, so I just added my code for altitude, latitude and longitude like that. While you're right with setting the pointer to NULL, maybe it would also be a good idea to initialize it to a defined value (NULL) in the first place. BTW: g_free() won't try to free NULL pointers, so the extra check won't be necessary. > On another note, the plugin still installs in libexec/xfce4, while now all > other plugins installs to lib/xfce4. I can send you the patch to fix that > and build the plugin as a module if you want. Ok, I missed that. But then, I also do know nothing about that. So yes, I'd appreciate such a patch very much. I think uploading files to the bug tracker should work again, so please either send it to me or upload it here, both will be fine. That said, now I can start wondering why it crashes for you with a double free in the first place, I didn't experience this during my tests with different locations. Thanks for your report.
Could it be that the glib implementation on OpenBSD handles memory management stuff a bit different and the code needs to care about it? I'm not very acquainted with non-Linux systems unfortunately...
Or it could also have to do with the version of libxml... Might I ask which version you use?
glib 2.32.2/libxml 2.7.8. And our glib itself has nothing special, but yes the mallocs funcs below it are a bit more strict than on linux.
Hmmm... i think i know. why do you xmlFree() altitude, latitude and longitude in parse_location, they're unused and uninitialized ?
Removing those 3 xmlFree() helps a bit, i can close the properties dialog and some data are fetched. But the plugin crashes when i try to open the forecast window or update the info through the right-click menu, so there might be other bugs. I'll debug it a bit more tmrw.
(In reply to comment #6) > Hmmm... i think i know. why do you xmlFree() altitude, latitude and > longitude in parse_location, they're unused and uninitialized ? You're right, I think that's a copy & paste error which did go unnoticed because the damn thing didn't crash on me. Now I'm also unsure about all that g_free stuff in that same function. It seems these can all be uninitialized, if one looks at the call to parse_location() in parse_time(). I believed these things would be dealt with somewhere else, so I never got suspicious about them before, especially as everything seemed to work fine. Maybe I should write proper init_location, init_time, init_whatever functions and use them after g_slice_new0()? That way it can be relied on that the members have defined values. IIRC I did something similar for the xfce_weatherdata struct.
But then again, g_slice_new0 should already initialize the members?
I removed the 3 xmlFree and pushed to git: http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=546f670c7f466887b08a724a71e84b8d9ebf8587 Currently I don't have enough time to work through the code, and since I don't get any crashes it's hard to debug. So I'd be very glad if you could try to find the problems or at least send me backtraces so I know where to start looking. So far, thanks for your help.
Created attachment 4570 Build the plugin as a module First, a patch to build as a module and install in more appropriate dirs (ie libdir, and panel/plugins instead of panel-plugins). I've made some progress on the crashers : When right clicking and forcing an update, it crashes with Program received signal SIGSEGV, Segmentation fault. xml_time_free (timeslice=0x0) at weather-parsers.c:268 268 xml_location_free(timeslice->location); (gdb) bt #0 xml_time_free (timeslice=0x0) at weather-parsers.c:268 #1 0x07af2bf7 in xml_weather_free (data=0x823cd000) at weather-parsers.c:279 #2 0x07aeca81 in cb_update (succeed=1, result=0x83b6f000 <Address 0x83b6f000 out of bounds>, len=142041, user_data=0x8bbdc018) at weather.c:383 #3 0x07af1dfd in weather_http_receive_data_destroyed (user_data=0x87c946c0) at weather-http.c:450 At that point, timeslice was already free()'d, maybe by xfceweather_free(). So in xml_weather_free() you should check for it to be null before calling xml_time_free. diff --git a/panel-plugin/weather-parsers.c b/panel-plugin/weather-parsers.c index c0326fb..a251e48 100644 --- a/panel-plugin/weather-parsers.c +++ b/panel-plugin/weather-parsers.c @@ -276,6 +276,7 @@ void xml_weather_free (xml_weather *data) for (i = 0; i < data->num_timeslices; i++) { xml_time_free(data->timeslice[i]); } - xml_time_free(data->current_conditions); + if (data->current_conditions) + xml_time_free(data->current_conditions); g_slice_free (xml_weather, data); With that patch, i can force updates and it doesnt crash at that point. But it still crashes when bringing the summary window : Program received signal SIGSEGV, Segmentation fault. localsub (timep=0x8, offset=0, tmp=0x7c20e9c0) at /usr/src/lib/libc/time/localtime.c:1273 1273 const time_t t = *timep; (gdb) bt #0 localsub (timep=0x8, offset=0, tmp=0x7c20e9c0) at /usr/src/lib/libc/time/localtime.c:1273 #1 0x0ea097f7 in localtime_r (timep=0x8, p_tm=0x7c20e9c0) at /usr/src/lib/libc/time/localtime.c:1366 #2 0x0ea09a1f in localtime (timep=0x8) at /usr/src/lib/libc/time/localtime.c:1380 #3 0x03342517 in create_summary_window (data=0x86fa2c18) at weather-summary.c:285 #3 0x03342517 in create_summary_window (data=0x86fa2c18) at weather-summary.c:285 285 point_tm = localtime(&conditions->point); It seems at that point conditions is empty/null/contains garbage. Strangely gdb says it doesnt know 'conditions' variable.. and data->weatherdata seems valid although data->weatherdata->current_conditions is NULL.. , so it seems make_current_conditions() wasnt called. (gdb) p get_current_conditions(data->weatherdata) $5 = (xml_time *) 0x0
So, after more gdb tracing, it seems all make_current_conditions calls return NULL, and it might come from find_timeslice() while{} loop never finding a 'valid timeslice' (has_timeslice keeps returning false) and in the end returns NULL. Note that even if i dont force an update or display the forecast window, the icon in the panel shows 'unknown' and i have no values displayed in the labels nor in the tooltip.
(In reply to comment #12) > So, after more gdb tracing, it seems all make_current_conditions calls > return NULL, and it might come from find_timeslice() while{} loop never > finding a 'valid timeslice' (has_timeslice keeps returning false) and in the > end returns NULL. That's probably because the timeslices have been freed before. xml_weather_free is called in xfceweather_free (IIRC this should only happen on plugin shutdown), in cb_update () and set_icon_error, nowhere else. All this happens in weather.c. set_icon_error gets called in various places and sets xml_weather to NULL. > Note that even if i dont force an update or display the forecast window, the > icon in the panel shows 'unknown' and i have no values displayed in the > labels nor in the tooltip. I'm a bit lost now. Do you get data once and then it gets lost, or don't you get any data at all?
Created attachment 4571 set-null-after-free.patch This patch should also prevent double freeing and checks for NULL before freeing the members to prevent crashes.
With your patch, it doesnt crash when forcing an update, but still crashes when opening the forecast window. Apparently, i do get the data at each update because on stderr i have (wrapper:24237): weather-WARNING **: getting http://api.yr.no//weatherapi/locationforecastlts/1.1/?lat=48.794998;lon=3.272400 but it's more likely the parsing failing.. btw the plugin could use some DBG() macros, instead of using g_warning().
(In reply to comment #15) > With your patch, it doesnt crash when forcing an update, but still crashes > when opening the forecast window. Yes, it simply _hides_ the double free errors, in my opinion making debugging harder, getting you one step farther away from your goal instead of nearer to it. Let's better fix the cause for the double frees instead of hiding the errors. > Apparently, i do get the data at each > update because on stderr i have (wrapper:24237): weather-WARNING **: getting > http://api.yr.no//weatherapi/locationforecastlts/1.1/?lat=48.794998;lon=3. > 272400 This is only printed when weather.c sends the http request. It doesn't mean the data it got is useful. But if there are no more messages one can assume data fetching works correctly. > but it's more likely the parsing failing.. btw the plugin could use some > DBG() macros, instead of using g_warning(). This is what I suspect too, but to be sure we need to check the appropriate structs after parsing. That g_warning has been there for ages, and when I needed to debug the stuff I simply threw in my own g_prints. Not very professional but that was the easiest way for me to do that fit in my workflow. But you're right, some DBG messages would be really useful. Could you add a few DBGs or g_prints or whatever you prefer to parse_time or parse_location and see if it writes anything useful?
(In reply to comment #15) > With your patch, it doesnt crash when forcing an update, but still crashes > when opening the forecast window. The crash happens because there's no NULL check for conditions in create_forecast_window. Try this, it might fix it: diff --git a/panel-plugin/weather-summary.c b/panel-plugin/weather-summary.c index cc8eb39..fcce16d 100644 --- a/panel-plugin/weather-summary.c +++ b/panel-plugin/weather-summary.c @@ -641,7 +641,10 @@ create_summary_window (xfceweather_data *data) conditions = get_current_conditions(data->weatherdata); - rawvalue = get_data (conditions, data->unit_system, SYMBOL); + if (conditions) + rawvalue = get_data (conditions, data->unit_system, SYMBOL); + else + rawvalue = NULL; icon = get_icon (rawvalue, 48, is_night_time()); g_free (rawvalue);
Ok, looking at get_data, I see that's not the problem. The crash might happen later.
From further analysis, i found strange that need_conditions_update is only called if need_data_update succeds in update_weatherdata. If need_data is true it probably means that make_current_conditions() should be called with the new data just received. Otherwise it needs two calls to update_weatherdata when starting to actually get to the parsing. After a bit more debugging, i've found that the timeslices were all added, but all of them gets -1 for the end of the timeslice. That might explain why has_timeslice always fails. I think in the time_ts/time_tm/time_t renaming either some occurences were missed, or using foo_t for a variable name is not the wisest choice, since it's often used as a typedef and can lead to funny errors...
Oh, and i think i finally found the causes of all that mess. #define _XOPEN_SOURCE at the top of weather-parsers.c hides strptime() definition, which can lead to funny things : weather-parsers.c:108: warning: implicit declaration of function 'strptime' and my_timegm() plays silly games with TZ and such, and i'm not sure it's a good idea, nor what was the intent at the beginning. Not doing it and directly call mktime() works here. with the following diff: diff --git a/panel-plugin/weather-parsers.c b/panel-plugin/weather-parsers.c index c0326fb..2fe1e1d 100644 --- a/panel-plugin/weather-parsers.c +++ b/panel-plugin/weather-parsers.c @@ -15,8 +15,6 @@ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */ -#define _XOPEN_SOURCE - #ifdef HAVE_CONFIG_H #include <config.h> #endif @@ -119,9 +117,9 @@ void parse_time (xmlNode * cur_node, xml_weather * data) { xmlFree(start); xmlFree(end); - start_t = my_timegm(&start_tm); - end_t = my_timegm(&end_tm); - + start_t = mktime(&start_tm); + end_t = mktime(&end_tm); I get a fully functionning plugin, including the forecast window.
(In reply to comment #19) > From further analysis, i found strange that need_conditions_update is only > called if need_data_update succeds in update_weatherdata. If need_data is > true it probably means that make_current_conditions() should be called with > the new data just received. Otherwise it needs two calls to > update_weatherdata when starting to actually get to the parsing. I don't know exactly what you mean, but the way it should work is this: Every 15 seconds check if update is necessary, calling weather_update. If latitude or longitude is unknown, no update will be necessary at all. Otherwise check if it is necessary to download new data. If it is do so and read in data (every 3 hours), then generate current conditions. If it is not, then check if we need to update current conditions (every hour). If it is necessary, regenerate current conditions. If not, wait another 15 seconds... > After a bit more debugging, i've found that the timeslices were all added, > but all of them gets -1 for the end of the timeslice. That might explain why > has_timeslice always fails. I think in the time_ts/time_tm/time_t renaming > either some occurences were missed, or using foo_t for a variable name is > not the wisest choice, since it's often used as a typedef and can lead to > funny errors... I checked it, using it is safe and the way it is done now is less confusing and more uniform. BTW: I could have used only time_t in most parts, even calculating with it, but it is not defined as an integer on every platform and that could certainly lead to undefined behaviour. Therefore, I replaced the code converting between struct tm and time_t.
(In reply to comment #20) > Oh, and i think i finally found the causes of all that mess. > > #define _XOPEN_SOURCE at the top of weather-parsers.c hides strptime() > definition, which can lead to funny things : > > weather-parsers.c:108: warning: implicit declaration of function 'strptime' Ok. What causes a warning on your system solves it on mine: http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=f44666af1ba7ea87e7b34072b403821194bd70df So I guess we should place other #defines around that #define _XOPEN_SOURCE for OpenBSD/Linux/whatever. Do you think FreeBSD is also affected? > and my_timegm() plays silly games with TZ and such, and i'm not sure it's a > good idea, nor what was the intent at the beginning. Not doing it and > directly call mktime() works here. I'm also not sure what its exact purpose is, and my reasoning was that it replaced functionality not available on non-Linux platforms. That's why I did not replace it. And of course it worked fine on my Linux system too. Remember the rule: Never change a running system. But if you tested it and mktime works fine and gives the correct results, I will happily adapt the code and get rid of that function. > I get a fully functionning plugin, including the forecast window. Fine and congratulations! Just to be safe, though: You might check that the calculated start and end times are correct and correspond to the XML data downloaded (before parsing). The algorithm used to find appropriate data might play recovery in case the exact times are not found. Please note: I'm not all done with cleaning and verifying. I just tried to get the plugin into a usable state so it can be released. There are certainly some issues I may not have discovered yet.
And now to the patch you uploaded for building the plugin as a module: Thank you very much, but does it generate static libraries? Am I right that the entries for libweather.la refer to those "*.la" files which my distribution (and others too IIRC) makes every effort to get rid of? There's even an lafilefixer tool for that. If so, isn't there a way to build the library dynamically? Or is it necessary on other platforms that the plugins are build this way? In fact, I have only .so files in my /usr/lib/xfce4/panel-plugins, or executable files: libdatetime.so libnotes.so libnotes.so.0 libnotes.so.0.0.0 libquicklauncher.so libsmartbookmark.so xfce4-cpufreq-plugin xfce4-eyes-plugin xfce4-mixer-plugin xfce4-netload-plugin xfce4-places-plugin xfce4-sensors-plugin xfce4-timer xfce4-verve-plugin xfce4-weather-plugin
(In reply to comment #22) > (In reply to comment #20) > > Oh, and i think i finally found the causes of all that mess. > > > > #define _XOPEN_SOURCE at the top of weather-parsers.c hides strptime() > > definition, which can lead to funny things : > > > > weather-parsers.c:108: warning: implicit declaration of function 'strptime' > > Ok. What causes a warning on your system solves it on mine: > http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/ > ?id=f44666af1ba7ea87e7b34072b403821194bd70df > > So I guess we should place other #defines around that #define _XOPEN_SOURCE > for OpenBSD/Linux/whatever. Do you think FreeBSD is also affected? I think user-code shouldnt have to fiddle with it. What was the warning on your system ? The linux manpage for mktime() doesnt mention that you need to define it, only including <time.h> should be enough. (In reply to comment #23) > And now to the patch you uploaded for building the plugin as a module: > > Thank you very much, but does it generate static libraries? Am I right that > the entries for libweather.la refer to those "*.la" files which my > distribution (and others too IIRC) makes every effort to get rid of? There's > even an lafilefixer tool for that. If so, isn't there a way to build the > library dynamically? Or is it necessary on other platforms that the plugins > are build this way? It generates libweather.so, and that's all we need to get installed. It's up to the packager/builder to pass the appropriate configure/libtool flags to only install it (i think it defaults to disable-static, see LT_INIT([disable-static]) in configure.ac.in) , and not build the static lib/la file. All the other plugins have the same code (i copied it from the xfce4-panel/plugins examples), the libweather_la file in Makefile.in is just a shorthand when building the .so. > In fact, I have only .so files in my /usr/lib/xfce4/panel-plugins, or > executable files: > > libdatetime.so > libnotes.so > libnotes.so.0 > libnotes.so.0.0.0 > libquicklauncher.so > libsmartbookmark.so > xfce4-cpufreq-plugin > xfce4-eyes-plugin > xfce4-mixer-plugin > xfce4-netload-plugin > xfce4-places-plugin > xfce4-sensors-plugin > xfce4-timer > xfce4-verve-plugin > xfce4-weather-plugin That's the old location, new-style location is /usr/lib/xfce4/panel/plugins
(In reply to comment #24) > (In reply to comment #22) > > (In reply to comment #20) > > > Oh, and i think i finally found the causes of all that mess. > > > > > > #define _XOPEN_SOURCE at the top of weather-parsers.c hides strptime() > > > definition, which can lead to funny things : > > > > > > weather-parsers.c:108: warning: implicit declaration of function 'strptime' > > > > Ok. What causes a warning on your system solves it on mine: > > http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/ > > ?id=f44666af1ba7ea87e7b34072b403821194bd70df > > > > So I guess we should place other #defines around that #define _XOPEN_SOURCE > > for OpenBSD/Linux/whatever. Do you think FreeBSD is also affected? > > I think user-code shouldnt have to fiddle with it. What was the warning on > your system ? The linux manpage for mktime() doesnt mention that you need to > define it, only including <time.h> should be enough. weather-parsers.c: In function 'parse_time': weather-parsers.c:106:55: warning: comparison between pointer and integer [enabled by default] weather-parsers.c:112:51: warning: comparison between pointer and integer [enabled by default] I've looked that up, don't know where, but it shouldn't be too hard to find, and was told that's because some prototype's missing, so comparison with NULL causes this warning. Nothing severe, but fixed by that #define. And the #define was there before, just at the wrong place, so it did not work. > (In reply to comment #23) [...] > It generates libweather.so, and that's all we need to get installed. It's up > to the packager/builder to pass the appropriate configure/libtool flags to > only install it (i think it defaults to disable-static, see > LT_INIT([disable-static]) in configure.ac.in) > , and not build the static lib/la file. All the other plugins have the same > code (i copied it from the xfce4-panel/plugins examples), the libweather_la > file in Makefile.in is just a shorthand when building the .so. Ok. I haven't tried it yet, but it will be fine. [...] > That's the old location, new-style location is /usr/lib/xfce4/panel/plugins Yes, you're right, I found this directory too with lots of .so files.
man strptime here on Linux tells me this: NAME strptime - convert a string representation of time to a time tm structure SYNOPSIS #define _XOPEN_SOURCE /* See feature_test_macros(7) */ #include <time.h> So I guess it's needed.
(In reply to comment #26) > man strptime here on Linux tells me this: > > NAME > strptime - convert a string representation of time to a time tm > structure > > SYNOPSIS > #define _XOPEN_SOURCE /* See feature_test_macros(7) */ > #include <time.h> > > So I guess it's needed. Then it also needs #define _XOPEN_SOURCE_EXTENDED 1, since strptime() is in XPG4v2. See for precedents : https://mail.gnome.org/archives/commits-list/2012-May/msg06151.html https://mail.gnome.org/archives/commits-list/2012-May/msg05632.html And it only needs to be before time.h i think. I find it really strange that you had to put it above all headers.. can you try putting it before weather-parsers.h inclusion, and then just before libxfce4panel.h ? maybe one of those already includes time.h diff --git a/panel-plugin/weather-parsers.c b/panel-plugin/weather-parsers.c index c0326fb..f86e8ba 100644 --- a/panel-plugin/weather-parsers.c +++ b/panel-plugin/weather-parsers.c @@ -15,14 +15,15 @@ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */ #define _XOPEN_SOURCE +#define _XOPEN_SOURCE_EXTENDED 1 #ifdef HAVE_CONFIG_H #include <config.h> #endif #include "weather-parsers.h" #include <libxfce4panel/libxfce4panel.h> With that, no warning for me when building weather-parsers.c
(In reply to comment #27) > See for precedents : > https://mail.gnome.org/archives/commits-list/2012-May/msg06151.html > https://mail.gnome.org/archives/commits-list/2012-May/msg05632.html Exactly what I needed but have not found. > And it only needs to be before time.h i think. I find it really strange that > you had to put it above all headers.. can you try putting it before > weather-parsers.h inclusion, and then just before libxfce4panel.h ? maybe > one of those already includes time.h It needs to be before weather-parsers.h. weather-parsers.h includes glib.h. Putting it after that and before libxfce4panel.h causes the warning to reappear. [...] > With that, no warning for me when building weather-parsers.c No warning here either, and everything seems to work fine. I'll look at the rest and investigate for other necessary NULL pointer fixes and commit tomorrow. Thanks for the fixes.
I thought a bit more about my_timegm(), and from my understanding it tries to convert the given struct tm to a time_t, but at the same time convert it as GMT/UTC time (since it unsets TZ). Maybe it tries to replace the declared as deprecated timegm().. maybe a clever call to mktime(gmtime(xxx)) would do the trick there.
I've looked it up, it's a portable version of timegm: Look at this page, especially the notes: http://www.kernel.org/doc/man-pages/online/pages/man3/timegm.3.html So it seems your problem was only caused by the missing #define.
To detect double-free attempts and prevent crashes, I think I found a better method: http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=a7eafaaac94476c0a6143ded7883b5bf5b82d9f3
The problem with the missing #define should be solved in http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=94c7c6e88ed7caf1ed8fec608558050274064a84
I've committed your patch for building the plugin as a module to the git repository (http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=77546c193fb1307c76981e8f41687687344b24f2). However, I had to slightly change the desktop.in file so the panel doesn't look for an executable: [Xfce Panel] Type=X-XFCE-PanelPlugin _Name=Weather Update _Comment=Show current weather conditions Icon=xfce4-weather X-XFCE-Internal=FALSE X-XFCE-Module=weather It works fine, thanks for your patch.
(In reply to comment #33) > I've committed your patch for building the plugin as a module to the git > repository > (http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/ > ?id=77546c193fb1307c76981e8f41687687344b24f2). > > However, I had to slightly change the desktop.in file so the panel doesn't > look for an executable: > [Xfce Panel] > Type=X-XFCE-PanelPlugin > _Name=Weather Update > _Comment=Show current weather conditions > Icon=xfce4-weather > X-XFCE-Internal=FALSE > X-XFCE-Module=weather Yes, i forgot to commit that part. Unfortunately, with a current clean git tree the plugin still does shit with end_tm: with start_tm : Breakpoint 1, my_timegm (tm=0xcfbdb590) at weather-parsers.c:33 33 { (gdb) p *tm $2 = {tm_sec = 0, tm_min = 0, tm_hour = 9, tm_mday = 25, tm_mon = 6, tm_year = 112, tm_wday = 3, tm_yday = 206, tm_isdst = -2102571200, tm_gmtoff = -2101606400, tm_zone = 0x33e37b "\201ÃyUÿ\037\201ì\214"} 40 ret = mktime(tm); (gdb) p ret $4 = 1343206800 with end_tm : Breakpoint 1, my_timegm (tm=0xcfbdb564) at weather-parsers.c:33 33 { (gdb) p *tm $6 = {tm_sec = 0, tm_min = 0, tm_hour = 9, tm_mday = 25, tm_mon = 6, tm_year = 112, tm_wday = 3, tm_yday = 206, tm_isdst = 9, tm_gmtoff = 2140462496, tm_zone = 0x2740dc98 "ô«"} 40 ret = mktime(tm); (gdb) p ret $7 = -1 Note the differences in tm_isdst and tm_gmtoff in the tm structs... If i look at manpages : http://linux.die.net/man/3/strptime 'In principle, this function does not initialize tm but only stores the values specified. This means that tm should be initialized before the call. Details differ a bit between different UNIX systems. The glibc implementation does not touch those fields which are not explicitly specified, except that it recomputes the tm_wday and tm_yday field if any of the year, month, or day elements changed.' http://www.openbsd.org/cgi-bin/man.cgi?query=strptime ' There is no way to specify whether Daylight Saving Time is in effect when calling strptime. To use the resulting tm structure with functions that check the tm_isdst field, either set it to a negative value, which will cause mktime(3) to attempt to divine whether Daylight Saving Time would be in effect for the given time, or compute the value manually.' So it turns out one has to zero out (with bzero() or memset()...) the struct tm if you want to user mktime after strptime, otherwise you rely on uninitialized values. I've tried with and without assigning -1 to tm_isdst and the behaviour seems the same. So here's (hopefully) the final fix : diff --git a/panel-plugin/weather-parsers.c b/panel-plugin/weather-parsers.c index d574df2..e608123 100644 --- a/panel-plugin/weather-parsers.c +++ b/panel-plugin/weather-parsers.c @@ -28,6 +28,7 @@ #include <libxfce4panel/libxfce4panel.h> #include <time.h> #include <stdlib.h> +#include <string.h> static time_t my_timegm(struct tm *tm) { @@ -97,6 +98,10 @@ void parse_time (xmlNode * cur_node, xml_weather * data) { time_t start_t, end_t; xml_time *timeslice; xmlNode *child_node; + memset(&start_tm, 0, sizeof(struct tm)); + memset(&end_tm, 0, sizeof(struct tm)); + start_tm.tm_isdst = -1; + end_tm.tm_isdst = -1;
Compiles fine, no warnings and the plugin works, so pushed to git: http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=654ce328524c1f9c7e812c1c999e8b51a2c4721c
After introducing astrological data, I've run into problems with the linux man page my_timegm(), so I replaced it with something different which should be portable too and hopefully works correctly: http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=c5e86912a6661c6a7b04d66c786693a01a2d45ea
Hm, I might have been mistaken, and the man page version works fine and the lock/crash was caused by a double free I've corrected with http://git.xfce.org/panel-plugins/xfce4-weather-plugin/commit/?id=d2496aeae3c2d56d859ea2a7a5400f97ecc11666. That would explain why it was somehow repeatable and only happened with location set to Tokyo. I will have to watch this for some time longer.
I'll close this as resolved fixed. If you still have issues in >=0.8.1, feel free to reopen. Bug #9182 comment #7 might also be of interest in that case.