Need to fix xmms plugin "memory leak"
I have found quite the wonderful xmms plug called "iNetctl". It allows me to stream the current track name from my linux box to my ibook, which I then format to my liking. I would like this because I listen to alot of streaming radio, and xmms/mpg123 can update the track info which iNetctl allows me to read outside of xmms (I have done it with vlc as well, but its so kludgy, involving using pty/tty pairs and a failed attempt of transferring the code from OSX to Linux.) It even takes care of the server part. I have even been able to create a sort of bi-directional script to control xmms through the iNetctl session from OSX as well (A whole lot of bash + pipes :D).
The problem though, is that the inetctl client controlling server [Henceforth: CCS] eventually eats up memory by the boatload. By boatload, I mean, 22 hours of play and the CCS has taken over 25% of my 256 mb ram (50 megs). A little more, and it would crashed xmms, and my x-session (It has once already). Here I'm wanting to use this on a 128mb system (Which would have DamnSmallLinux loaded to ram as well). Xmms normally takes about 6~10 megs, and the iNetCtl listening server has stayed at 3~5mb. The CCS starts out at 3~5mb as well, but eventually starts ballooning up.
I have looked through the code for the iNetCtl, and only see 2 instances of memory creation and resizing. I have tried adding mtrace(), and running it, but for some reason, the mtrace output gets huge (with glib allocs) And huge, I mean 50 megs in 4 minutes. The /usr/bin/mtrace gives me a bunch of "Interger overflow in /usr/bin/mtrace line x" errors as well, so I can't use that to find why the CCS eats so much memory. I run out of ram/harddrive space before it has a chance to get even a megabyte bigger.
So, before I babble on some more, the first thing is to see if you guys can recreate it as well. I am using DSL 3.4.11, which has XMMS 1.2.8. You need the iNetCtl source code (from SourceForge) as well as the XMMS-dev files (I used the 1.2.7 dev files from a debian repo). The configure takes no options, and Make produces only one file (libinetctl.so) which can go into ~/.xmms/Plugins/ . After that, just enable the plugin, load up a streaming web station of your choice, and connect to the iNetctl server (nc localhost 1586 > /dev/null) (I don't know of a way of bg the process, so it will take over a terminal session, or use "screen"). If you check ps for its memory size, then come back in about 4 or 5 hours, you will see it should have doubled. 22 hours inflated mine from 5mb to 50mb. (~2mb per hour).
ps -avfx should look like this
The only malloc it uses (in function parseArgs [inetctl_command.c line 97]) is for the buffer size (a string length, <128 characters, afaik), and resized if the buffer size is bigger then that. It looks like it just gets reused, resized/remalloced only if its needed, and that's it. parseArgs is only called when the client sends a command, and the way I have been using it hasn't sent any commands other then "time_updates off" sent once. The other 22+ hours have been spent reading the track info that it automatically sends. There's no free() call. Could that be it?
Any help would be greatly appreciated.
What you are doiong is a good way to get your feet wet with C. It's exactly what I have been doing for a couple of years. I'm still no C programmer, though, but mabe this patch work:
--- xmms-inetctl-1.2/inetctl_command.c.00 2008-05-29 10:50:31.000000000 +0200
+++ xmms-inetctl-1.2/inetctl_command.c 2008-05-29 10:50:44.000000000 +0200
@@ -197,6 +197,10 @@
inParm = FALSE;
+ /* free the buffers */
/* At EOL, make sure we are not in quote mode */
That compiles okay on my Slackware system and seems to be what is indicated -memory must be freed after using alloc, malloc or realloc.
See VALGRIND for debugging and diagnostics. It is made up of a few tools to find potential problems in existing code. I have used it with success to track memory leaks.
You can also add a global counter to the call that does the memory allocations and freeing, so that you can track how many different allocation are leaking the memory.
If the code has not been written with a valid concept for proper memory usage, then it will be quite painful to modify it afterwards - you will need a lot of testing to avoid breaking the code.
The source code for the plugin in question is quite small -I simply did this in the sources:
grep -nrH alloc *
It showed up only one place where alloc is called:
parseBuffer = (String) malloc(parseBufferSize);
and one where realloc is called:
parseBuffer = realloc(parseBuffer, parseBufferSize);
I then grepped for 'free':
grep -nrH free *
which only shows two occurrences of 'xmms_cfg_free(config);'
and two for 'g_free(configfile);'
Strangely, there appear to be no memory allocations asociated with these. Anyway, I just went into the function where the alloc and realloc are located and put the free commands as (nearly) the last thing done in that function. Maybe it will work -it does compile alright and surely won't hurt anything. Maybe someone will chip in to the thread if that isn't right and tell us what is...
Thanks for the reply guys.
I have added gnashley's patch (manually :P) and compiled, but I received a gcc warning:
/* free(parseBufferSize); */
As parseBufferSize is never malloc'd. It is just used to hold the size variable for the malloc and remalloc.
Just recompiled, and will be giving it a go.
Oh, and I was "watch"ing the process in ps, and have found that it increases in memory by 4kb roughly every 7 seconds. Doing the math, that's 2057kb per hour, the ~2mb I had guesstimated earlier.
Well, I started it nearly 6 and a half hours ago, and it has continued to eat those 4kb regardless of the patch.
It starts out at
What can I try now?
Edit: Running out of memory sure did help. I was untaring the valgrind package (4mb bz2 tar, into 31mb) and I run out of memory.
__alloc_pages: 0-order allocation failed (gfp=0x1d2/0)
Vm killed tar, and apparently shrunk every other process down some.
The gui xmms processes went down to 4mb from 8mb, the inetctl listening server down to 1.5mb, and the previously 18.5mb (7.7%) CCS to 15.4mb (6.4%).
The CCS did not crash or stop working correctly, afaik. Interesting. Can that memory starved message be forced?
New update. Interestingly enough, the patch has seemed to break the plugin. I could not send commands to it over telnet anymore. It responded with missing command, then unknown command, even though they were valid commands, and froze up my telnet on ^C. I had to use ssh to login and kill the process.
The search goes on.
I seem to have found the source of the leak. Well, with the help of a debian bug report for a similar xmms plugin, xmms-infopipe 1.3, which inetctl so happens to be based on.
The problem is a mainly, a pair of char's in inetctl_status.c, and some gint's and gboolean in there as well. They are all defined in inetctl.h, line 71, the playerstatus structure. They are then first used in inetctl_status.c line 18, the function getPlayerStatus. The structure is initialized and filled when "xmms_remote_*" functions are called. The two biggest culprits, I think, are line 26 strcpy(theStatus->trackTitle, xmms_remote_get_playlist_title and line 27 strcpy(theStatus->trackFile, xmms_remote_get_playlist_file.
Those two are char types, according to the inetctl.h, but the xmmsdev files define them as gchars. :/
Anyway, by disabling line 26, 179, 180, and 304 in inetctl_status.c, and line 83 in inetctl.h (all instances and uses of the titletrack char) and running xmms with the modified inetctl, I have greatly reduced the memory eating, from 4kB ~ 7 seconds to 4kB every 39/40 seconds, which is just about 370kB (vs 2mB). Bad part is, it completely ruins the whole idea for using inetctl in the first place. No streaming track title :(
So, how can I fix this? A small leak like this (~7mb per day instead of 48mb per day) is better but not preferred. 3mb per day would be okay. None would be best :D.
The best way is to find out how those structures are being used and when they can be safely destroyed. Any memory leak in a program which runs for extended periods of time is simply intolerable. Imagine if Apache had leaks ... servers would be crashing as if they were running WinDuhs.
Look around, there might be some graphical tool that helps you go through a core dump. If you find a good one, you can force a core dump then inspect the allocated pointers/memory to see what's eating things up, then grep the source to see where they are allocated, and inspect the source to see where they are used, and if they are ever deallocated. The procedure goes something like this:
compile the leaky code with the "-g" option to preserve debug symbols
ulimit -c 4000000 (allow just a bit under 4MB for a core dump)
run the buggy software in the background - for example:
after a while kill the buggy software with a segfault (or any number of signals, but segfault works nicely):
kill -SIGSEGV $(pidof buggything)
That should leave you with a core file to play with.
Hmm... on the other hand - you said there is only one call to malloc/realloc?
You need to check the code then to see if it calls any routines within XMMS which do call 'malloc' and expect the caller to free the memory. Typically these are routines which return pointers like "char *blah(int one)" or routines which pass handles:
"void blah(char **somestring)"
'inetctl' seems to be 'abandonware' though; if you're really bent on using it and have no alternative I'll have a quick look at it when I'm bored.
It's just 6 *.c files and one header -maybe you could have a quick look -it doesn't look to be overly complicated.
I haven't found a debugger to use yet, but I have a core dump. (Even though I set ulimit -c 4000000, it dumps to 6.5mb (1.3mb according to du -h) I had to use kill -3 so only the CCS would dump, instead of every instance of xmms, and because kill -11 didn't produce a dump (Though I think that's because the program was running before I used ulimit -c).
Since I haven't found/installed a debugger (Or learned to use it), I did the next best thing. I "string"ed it. Now, this instance was only running for about 4 minutes (It had grown maybe 40kb in memory), I had changed tracks to force some status changes, and it is currently patched to only request the track-filename instead of both track-name and track-filename (Since this is where the leak is, imho), yet there is 6939 instances of one track-filename, and 237 instances of the other track-filename.
I will test without disabling getting the track-title and run that for 10 minutes, then compare. BBl.
Edit: So I have uncommented out the lines, and compiled with -g. Only ran for 16 minutes, and it has less instances of the track-title or track-filename as before, but since the string is quite larger, I would chuck it up to that. So I think that that is where the leak is.
And, between reading here, the code, and the bug report for xmms-infopipe, I think I have figured out where to free the memory. I can't free the playerstatus structure called lastplayerstatus because it is used to compare all the time, but the structure "playerStatus curStatus;" inside of the function "updatePlayerStatus(int clientSocket)" (Line 262, inetctl_status.c) is a local structure that is recreated on each call, unlike the static lastplayerstatus. So this is where I think I need to free the char trackTitle and char trackFile.
Question is, since xmms defines that as a gchar (line 53,54 xmmsctrl.h), but inetctl defines it as a char (Line 83,84 inetctl.h) do I use free or g_free? Should I also use g_free on the gints as well?
Success! I have, knock on wood, fixed it!
Going with that debian bug fix, I first changed the inetctl.h file to reflect gchar instead of char, even though its the same thing, just for consistency's sake.
Next, I had tried to add g_free(curStatus.trackTitle) to function updatePlayerStatus(), to free the xmms_remote call the curStatus structure makes, but that did not work. Kept segfaulting. I had though it was because it used function getPlayerStatus() to create curStatus, but it only lead me to the right direction.
In getPlayerStatus, the request for the track title and track filename are different then the rest. instead of direct assignment, it used strcpy(). Combined with the debian bug info, and reading the xmmsctrl.c file that the function for the title is in, it turned out that strcpy could be used to read an gchar ARRAY and place it into something as a string. So, I did the most kludgy thing since using tee to make multiple copies of stdout (which is pointless, in hindsight, because you can redirect 1> multiple times :D )...
I created a local gchar array for both trackTitle and trackFile, assigned it to the correct xmms_remote, strcpy'd the local array to the structure and then g-free'd the local array. Instant leak killer. (Then again, this is almost verbatim what the debian bug report by firstname.lastname@example.org says to do)
I have been running the CSS for the last 20 minutes without a single kilobyte past 3460kB eaten or bit or ballon'd. Of course, I'm going to leave it running for the next day or 2 to see how it holds up :P
Oh goody, I can avoid looking at the code then.
Yes, you can make some improvements; at the moment you can get heap corruption with the 'strcpy'. Consider using 'strncpy' to limit what is copied (read the docs - you need to watch out for that terminating 0), or else use 'snprintf' (which will take care of the terminating 0 for you). In principle 'strncpy' will be faster, but you need the few extra lines of code to make sure there is a 0 terminator at the end of the string.
The end result is that if the file/track names are so long that they won't fit in your fixed array, they will be truncated rather than possibly overwriting data beyond what was allocated to the array.
|All times are GMT -5. The time now is 12:28 PM.|