View Issue Details

IDProjectCategoryView StatusLast Update
0003919unrealircdpublic2012-10-14 11:44
ReporterwargAssigned Toohnobinki 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformOSCentOSOS Version5.5
Product Version3.2.8 
Target VersionFixed in Version3.2.10-rc1 
Summary0003919: recursive inclusion of unrealircd.conf causes hang and excessive cpu usage
DescriptionIf 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 ReproduceCompile UnrealIRCd and include *.conf; in unrealircd.conf.
Additional InformationI've attached a screenshot of useful/relevant data.
TagsNo tags attached.
3rd party modules

Activities

2010-07-10 00:32

 

Screenshot.png (287,101 bytes)

ohnobinki

2010-07-14 04:03

reporter   ~0016149

I suppose the best way to fix this is to detect and error upon any include recursion.

syzop

2010-07-14 16:57

administrator   ~0016160

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.

warg

2010-07-14 22:42

reporter   ~0016192

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.

2010-07-18 21:01

 

unreal-recursive-includes-error.patch (15,038 bytes)

ohnobinki

2010-07-18 21:09

reporter   ~0016216

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. ;-)

2010-07-18 21:39

 

unreal-recursive-includes-error-r1.patch (15,038 bytes)

ohnobinki

2010-07-18 21:40

reporter   ~0016217

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.

syzop

2010-07-19 11:52

administrator   ~0016218

I see.

I've rescheduled it for 3.2.10 ;)

syzop

2012-10-06 12:14

administrator   ~0017148

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.

syzop

2012-10-13 14:15

administrator   ~0017159

postponed for now.
I'm working on 0004091 which will probably cause conflicts when both this & that fix merge. please do not commit.

syzop

2012-10-14 11:44

administrator   ~0017162

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).

Issue History

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