GNOME Bugzilla – Bug 331099
Implement CamelArgV and CamelArgGetV using flexible arrays
Last modified: 2013-09-10 13:42:14 UTC
Steps to reproduce: 1. Install evolution-data-server-1.4.2.1_1 and evolution-2.4.2.1 on FreeBSD/amd64. 2. Start evolution 3. Watch your disk thrash until you've filled your 2GB of swap and the OOM killer finally kicks in Stack trace:
+ Trace 66156
Other information: Frame #3 is at line 457 of camel-object.c, and "count" is 1. Workaround patch to follow.
Created attachment 59310 [details] [review] Fix the allocation by being slightly liberal. Attached patch fixes the problem for me. I suspect some issue with type promotion causing a negative (i.e. huge) size to be allocated, but I don't know the rules enough to say for sure.
I verified that on all 64-bit FreeBSD platforms (amd64, sparc64, and ia64), the following code results in sz being 68719476768: sz = (sizeof (*argv) + (count - CAMEL_ARGV_MAX) * sizeof(argv->argv[0])); If count == 1. The same happens on 64-bit Solaris, but instead of malloc'ing the memory, Solaris returns EAGAIN. On i386, sz is 20 in this example. Another thought I had about fixing this is explicitly casting the argument to g_try_malloc() to be a guint32. So: argv = g_try_malloc ((guint32) (sizeof (*argv) + (count - CAMEL_ARGV_MAX) * sizeof(argv->argv[0]))); By doing this, sz in the above example would be 20 on i386, and 48 on 64-bit FreeBSD architectures.
*** Bug 331208 has been marked as a duplicate of this bug. ***
bumping priority and targeting this to the next release. Assigning it to camel hackers for patch review.
I would rather put it like... argv = g_try_malloc ((gulong) (sizeof (*argv) + (count - CAMEL_ARGV_MAX) * sizeof(argv->argv[0]))); as per the glib documentations.
Created attachment 71764 [details] [review] Updated fix. Cast the output of the calculation to gulong, as per the glib documentation.
Did anyone have a chance to test this patch?
I won't be able to as in the 9 months since I filed this bug, I've stopped using that machine.
based on Joe's comments/patch, looks like Varadhan's patch should work fine, right? if so, I'd suggest committing.
Yes, this works, but there is another place in the same camel-object.c that needs attention. Attached is the local patch we are currently using.
Created attachment 80830 [details] [review] Local FreeBSD patch to fix allocation problems
But this last patch also changes the previous patch from gulong to guint32? Can we agree on one fix? :-)
Sorry I wasn't clear. The gulong cast works, but I just submitted my patch to show you the other place where the cast is needed.
Created attachment 81259 [details] [review] patch
Attached a new patch using gulong in those two cases too then. The updated patch has been commited so I marked it as such.
Any chance we could redefine CamelArgv to use a flexible array? Then we could do away with CAMEL_ARGV_MAX altogether and the allocation would simply be: g_try_malloc (sizeof(CamelArgV) + count * sizeof(CamelArg))
Matthew/Varadhan/Sankar: Guys any stand on the comment #14 and comment $16 ? Can we get one of this in for 2.11.2?
(In reply to comment #16) > Any chance we could redefine CamelArgv to use a flexible array? Then we could > do away with CAMEL_ARGV_MAX altogether and the allocation would simply be: > > g_try_malloc (sizeof(CamelArgV) + count * sizeof(CamelArg)) > That would be just great, but, it would result in ABI break. Neverthless, I think its a good idea to make it a flexible array. Matthew: got anything in mind?
Created attachment 88009 [details] [review] Interim patch I started on a patch for that late last year. I'll try to finish it. This is the patch we wound up using in RHEL 5. It keeps the intermediate values in the expression from going negative. There's two places where this happens.
(In reply to comment #19) > Created an attachment (id=88009) [edit] > Interim patch > > I started on a patch for that late last year. I'll try to finish it. > > This is the patch we wound up using in RHEL 5. It keeps the intermediate > values in the expression from going negative. There's two places where this > happens. > patch re-phrases the calculation in a better way - however, just thinking of why can't we make CamelArg a flexible array, as in both the cases we know the count of CamelArgs that we need.
Sure. I was offering the patch as a temporary fix for 1.10.2 until I or someone else can write the flexible array patch. Sorry if I wasn't clear on that.
Guys, lets commit the interim patch for stable and head and rephrase the bug then. Lets get this in before it becomes too late. This is an important bug IMHO.
Interim patch committed to trunk (revision 7813) and stable (revision 7814). The bug is now about making CamelArgV and CamelArgGetV use flexible arrays and doing away with the hard limit CAMEL_ARGV_MAX. Changing summary appropriately.
Any further progress in this? What is the call?
My plan for Camel is to convert CamelObject to a GObject subclass, deprecate most of the CamelObject API, and kill CamelArgV and CamelArgGetV entirely in favor of GObject properties. Probably won't happen until after 2.24, though. Once done, and if no one objects to my plan, I'll close this bug as OBSOLETE.
Closing now that this has been done.