View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0003919||unreal||ircd||public||2010-07-10 00:32||2012-10-14 11:44|
|Target Version||Fixed in Version||3.2.10-rc1|
|Summary||0003919: recursive inclusion of unrealircd.conf causes hang and excessive cpu usage|
|Description||If I do include *.conf;, unreal is trying to recursively include unrealircd.conf, so it hangs at * Loading IRCd configuration .. and ignores SIGINT.|
It may be stupid to include *.conf; (my opinion), this should not be expected behavior.
|Steps To Reproduce||Compile UnrealIRCd and include *.conf; in unrealircd.conf.|
|Additional Information||I've attached a screenshot of useful/relevant data.|
|Tags||No tags attached.|
|3rd party modules|
Screenshot.png (287,101 bytes)
||I suppose the best way to fix this is to detect and error upon any include recursion.|
I feel like this is indeed not a nice bug, if we can fix this for 3.2.9 that would be nice. I then mean fixing the accidental version of this bug.
It would be hard to fix a non-accidental thing, like if someone created a xx mb config file, or a remote include one which keeps including another url with a slight variation...
But the one mentioned in this bugreport getting fixed, yeah that would be nice. Like by compiling a list of includes already included (or scheduled to include) and not re-including them.
Question of throwing a warning or error comes up, what do you guys think?
The include *.conf would be nice to have without warning or error, or should we discourage it ?
If it takes too much effort for 3.2.9, then I won't delay the release for it though, I mean it's not Release Critical.
||I agree Syzop, i like the list idea, and i too don't believe it's worth stalling release. I stumbled onto this accidentally, and thought it would be worth the bug.|
unreal-recursive-includes-error.patch (15,038 bytes)
||oh, stupid me, that patch is broken. I meant to test the remote-includes stuff before posting it here. But, if anyone's interested in reviewing what's there that would be very useful. ;-)|
unreal-recursive-includes-error-r1.patch (15,038 bytes)
I wrote these notes while writing the patch:
> The include *.conf would be nice to have without warning or error, or should we discourage it ?
I think we should give a hard error on that if possible. The only way to avoid giving the user an error is, AFAICT, to pass over an include directive without actually performing the include. I would, as a .conf writer, prefer that an include directive always includes.
Users can easily get around over-general glob expressions by:
1. making a new subdirectory to put all of the non-unrealircd.conf *.conf files. Then use include "conf.d/*.conf";
2. using a somewhat less general glob expression. Such as include "mynet_*.conf";
Both of these options are cleaner than adding more *.conf files into the same directory as unrealircd.conf.
- add_include() and add_remote_include() already had a checks for whether or not a file was included, which silently ignores the doubly-included file. However, this check is too late because load_conf() is always called a line or two before add_include() is called. Also, add_include() and add_rmeote_include() are unable to return an error code to their caller or inform the user of the problem (at least with their current parameter/arugment signatures). And, yet again, I think it makes more sense to put the checks into load_conf() because otherwise there'd be more duplicated code between add_include() and add_remote_include()
I am quite convinced that any cleanups I did in this patch are quite essential:
- For example, to tell a user the filename and line number where the recursion occured, I had to store that info into ConfigItem_include. The asynchronous remote includes code only knows this information when queuing the download. Also, the code that checks if there is recursion requires that add_remote_include() only be called _once_ -- thus the birth of update_remote_include().
- I avoided one opportunity to recursively throw ``const'' around, adding it only when I added/changed parameters and only when no recursive const additions were needed.
- Setting INCLUDE_USED in conf_load() is necessary because add_include() is now called before conf_load() instead of afterwards.
- Perhaps I am abusing ConfigItem_include by putting CPATH ("unrealircd.conf") into it and by using the conf_include to detect recursino. Yet, it seems that the conf_include list is only used during a rehash and that the data stored there is never reused after a rehash is over. That is, ConfigItem_include naturally works for checking things like include recursion.
I am fine with this being called too clean-up-y as long as it has a chance of being added later ;-).
Unanswered question(s) I had after editing the code:
- I see complication in unload_notloaded_includes() and unload_loaded_includes() and load_includes(). Why does conf_include even need to be kept around _after_ the cofiguration is loaded? I think it's only used during rehashing. If it's only needed during rehashing, it seems that the list could be emptied after a rehash is completed. Then the INCLUDE_NOTLOADED and INCLUDE_USED flags could be removed.
I've rescheduled it for 3.2.10 ;)
How confident are you that the patch is OK. Was it well tested? Was it ran through a memory debugger? Or are there any issues you are unsure of or make you doubt (eg: risk of double-free or.. whatever..).
If confident, you can commit it (like I said in June) prior to going into alpha/beta/.. stage.
Otherwise, we can postpone it to a later release.
postponed for now.
I'm working on 0004091 which will probably cause conflicts when both this & that fix merge. please do not commit.
Patch applied cleanly, even after my other url.c changes. However had two compile errors, one missing ; and the other was a call to update_remote_include() with too many parameters. Fixed those, then had a crash in a function of s_conf where it was always refering to x->url without checking if x->url is non-NULL, fixed that one. Then it seemed to work, ran it through valgrind, tried a number of cases, including: simple local file, wildcard, remote include. and remote includes referring to local files, other remote includes, etc. didn't try A->B->C->A, but I presume that is catched as well...
Thanks for the patch & the report.
- Fix bug where recursive includes would hang the IRCd, patch from
binki with some minor modifications, reported by warg (0003919).
|2010-07-10 00:32||warg||New Issue|
|2010-07-10 00:32||warg||File Added: Screenshot.png|
|2010-07-10 02:00||ohnobinki||Relationship added||child of 0003776|
|2010-07-14 04:03||ohnobinki||Note Added: 0016149|
|2010-07-14 16:57||syzop||Note Added: 0016160|
|2010-07-14 16:57||syzop||Status||new => acknowledged|
|2010-07-14 22:42||warg||Note Added: 0016192|
|2010-07-17 00:30||ohnobinki||Status||acknowledged => assigned|
|2010-07-17 00:30||ohnobinki||Assigned To||=> ohnobinki|
|2010-07-18 21:01||ohnobinki||File Added: unreal-recursive-includes-error.patch|
|2010-07-18 21:09||ohnobinki||Note Added: 0016216|
|2010-07-18 21:39||ohnobinki||File Added: unreal-recursive-includes-error-r1.patch|
|2010-07-18 21:40||ohnobinki||Note Added: 0016217|
|2010-07-19 11:49||syzop||Relationship deleted||child of 0003776|
|2010-07-19 11:49||syzop||Relationship added||child of 0003915|
|2010-07-19 11:52||syzop||Note Added: 0016218|
|2012-10-06 11:41||syzop||Relationship deleted||child of 0003915|
|2012-10-06 12:14||syzop||Note Added: 0017148|
|2012-10-13 14:15||syzop||Note Added: 0017159|
|2012-10-13 14:15||syzop||Status||assigned => acknowledged|
|2012-10-14 11:44||syzop||Note Added: 0017162|
|2012-10-14 11:44||syzop||Status||acknowledged => resolved|
|2012-10-14 11:44||syzop||Fixed in Version||=> 3.2.10-rc1|
|2012-10-14 11:44||syzop||Resolution||open => fixed|