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 |
| Reporter | warg | Assigned To | ohnobinki | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| OS | CentOS | OS Version | 5.5 | ||
| Product Version | 3.2.8 | ||||
| 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. | ||||
| Attached Files | |||||
| 3rd party modules | |||||
|
|
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. |
|
|
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. ;-) |
|
|
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 see. 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. http://hg.unrealircd.com/hg/unreal/rev/db66aa54c26e - Fix bug where recursive includes would hang the IRCd, patch from binki with some minor modifications, reported by warg (0003919). |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 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 |
