GNOME Bugzilla – Bug 732839
Long error messages cause DBus to abort() process
Last modified: 2014-09-02 08:50:56 UTC
Created attachment 280061 [details] Patch to limit error message length in cache_insert_metadata_decomposed When a very long error message is eventually sent over D-Bus and the original method call involves file descriptor passing, it hits bug 732615 and the sending process gets aborted. This is particularly the case of errors on updating nie:plainTextContent where the value may be megabytes long. Attached is a naive patch fixing just a single instance of this issue. Most likely there is more of them and it is also not only problematic with respect to the mentioned gdbus bug - e.g. system logs can grow quickly when things go bad.
Wow, good catch Martin. I think this is related to several reports recently related to miner-fs crashes and I remember seeing dbus in the stack trace a few times. It's a hard one to catch too, well done. For now, I will mark the other bug reports as a duplicate because I know we're producing errors for files that can't be extracted and it's likely that starts this whole thing off. I will review the patch as soon as I can! Thanks again!
OK, so I found some time to review this patch. IT looks great thank you for that. My main concern was the allocation of memory actually, but then I realised this is an error condition so we should be OK and it's not expected. I like your patch, I've applied it with some small changes (code style) and mentioned the bug in the patch comments. libtracker-data: Limit error message length When a very long error message is eventually sent over D-Bus and the original method call involves file descriptor passing, it hits Gnome bug 732615 and the sending process gets aborted. https://bugzilla.gnome.org/show_bug.cgi?id=732839 I will be doing an unstable and stable release this week, so it should be in 1.0.3 and 1.1.0. Thanks again! :)
*** Bug 733438 has been marked as a duplicate of this bug. ***
*** Bug 733353 has been marked as a duplicate of this bug. ***
Bug in launchpad.net : https://bugs.launchpad.net/ubuntu-gnome/+bug/1342923
*** Bug 733673 has been marked as a duplicate of this bug. ***
*** Bug 734211 has been marked as a duplicate of this bug. ***
About this solution: https://github.com/Igalia/nemo-tracker/commit/bb5dcb47cd78ec06e4ef67f81859feece8e40627#diff-d41d8cd98f00b204e9800998ecf8427e My review. Please DO read this. I'm very sorry. The commit is technically completely right. But I will not accept this because it introduces a malloc at a critical point in the performance of Tracker's SPARQL INSERT engine. Please refactor the code so that the EXACT place where the UTF8 truncation is needed is the one that performs the truncation and NOT just any field. Because this way, due to the fact that you are doing the malloc() at cache_insert_metadata_decomposed (which is used awfully a lot by the SPARQL INSERT stuff: for each and every triple being inserted), I can't accept. I would let you make Juerg's code slower. I can't do that. It's for me not right. Unless you get Juerg's approval on this one. Sorry.
Please revert that commit and discuss it further on the mailing list.
It's ok. I discussed this with Juerg and it's a code path that only executes in a error condition. There is a way to optimize this by stealing it out of the GValue because g_value_transform already does what is being done. Can I recommend rewriting the patch to attempt doing that? But because it's a error path of the code. It's ok. I'm putting this back into resolved fixed. My alarms went off because you touched the most performance critical path of Tracker's SPARQL INSERT engine. It's my task as co-maintainer to watch that stuff ;-)
(In reply to comment #10) > It's ok. I discussed this with Juerg and it's a code path that only executes in > a error condition. Hi Philip, yes that's why I allowed the memory allocation in the first place, it's an uncommon situation. > There is a way to optimize this by stealing it out of the > GValue because g_value_transform already does what is being done. Can I > recommend rewriting the patch to attempt doing that? And how would you then limit the length of the string that g_value_transform() produces? The main issue here is the size of the string that we send over DBus and null terminating a multibyte? string at some index (possibly mid-character) is probably worse than using the existing solution. I actually am not sure if the 255 limit is character length or string length, but our solution uses character length and that can clearly by bigger with UTF8 characters. > But because it's a error path of the code. It's ok. I'm putting this back into > resolved fixed. > > My alarms went off because you touched the most performance critical path of > Tracker's SPARQL INSERT engine. It's my task as co-maintainer to watch that > stuff ;-) No problem, my comment #2 said the same thing ;)