Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 203915
Summary: | 64-bit unclean code in evolution | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Paul F. Johnson <paul> | ||||
Component: | evolution | Assignee: | Matthew Barnes <mbarnes> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | |||||
Severity: | high | Docs Contact: | |||||
Priority: | high | ||||||
Version: | 6 | CC: | desktop-bugs, gnomeuser, jakub, redhat | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | x86_64 | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | evolution-data-server-1.8.0-11.fc6 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-09-27 14:37:51 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 150224 | ||||||
Attachments: |
|
Description
Paul F. Johnson
2006-08-24 13:58:37 UTC
There seem to be several bugs. One is that evolution calls malloc (0x1000000030). camel_object_get_type has (%rax is 1): lea -20(%rax), %edi ! Note, 32-bit result shl $0x4, %rdi add $0x160, %rdi and passes that as argument to g_try_malloc. This corresponds to: 426 guint32 i, count, version; ... 459 /* we batch up the properties and set them in one go */ 460 if (!(argv = g_try_malloc (sizeof (*argv) + (count - CAMEL_ARGV_MAX) * sizeof (argv->argv[0])))) 461 return -1; which is clearly 64-bit unclean code. Either you want to allocate at least sizeof (*argv), then it should be (MIN (count, CAMEL_ARGV_MAX) - CAMEL_ARGV_MAX) * sizeof (argv->argv[0]) or something similar, or it wants to allocate sometimes less than sizeof (*argv), in that case it could use something like offsetof (CamelArgV, argv) + count * CAMEL_ARGV_MAX. Or use signed count. Another one is glibc behavior when malloc is called with this and there isn't enough memory, will look at that now. Seems to be happy with the glibc update on 26th Aug. Okay, it's not crashed and burned. Closing the bug This shouldn't be closed until evolution side is fixed. Perhaps it doesn't crash or hang if it the malloc fails, but the 64-bit unclean code is still causing an allocation which will very likely not succeed ever. Matt, did you investigate the question in comment #1 ? Do we want to allocate at most (but sometimes less than) CAMEL_ARGV_MAX, or what is the goal here ? It appears to me that we should maybe change the array in CamelArgV to be flexible, and do away with CAMEL_ARGV_MAX altogether ? Then we would simply do g_try_malloc (sizeof(CamelArgV) + count * sizeof(CamelArg)) Wow, what a mess. Why couldn't they just use GValueArray for this stuff? I don't see how CAMEL_ARGV_MAX is actually limiting anything as far as memory allocation goes. It looks like the following expression is intended to come out negative: (count - CAMEL_ARGV_MAX) * sizeof (argv->argv[0]) But, for example, if count == (CAMEL_ARGV_MAX + 1) it works out to: (1) * sizeof (argv->argv[0]) which is positive. So the allocation limit is bypassed. I spotted two other places in camel-object.c that use this expression. I forgot to clarify that the intent of the expression seems to be to allocate just enough memory to hold a given number of CamelArgs. I like Matthias' idea of using a flexible array, but I think I'll save that for post-FC6 (eventually I'd like to rewrite the entire Camel vararg mechanism). For now I'll take a more conservative approach that actually enforces the limit: count = MIN (count, CAMEL_ARGV_MAX); argv = g_try_malloc (sizeof (CamelArgV) - (CAMEL_ARGV_MAX - count) * sizeof (CamelArg)); Created attachment 137194 [details]
Patch to clean up 64-bit unclean code
Fixed in evolution-data-server-1.8.0-11.fc6 Retest with evolution-2.8.0-6.fc6, evolution-data-server-1.8.0-11.fc6 and glibc-2.4.90-36 by the following steps: 1. Launch evolution via command or Email icon on panel 2. Normally use it, such as setup account,new message, send/receive mails, etc. 3. Observe CPU history and other system resouces. Test result: There was no abnormity showed as evolution can be used normally and system resouces load was low. |