0003919: recursive inclusion of unrealircd.conf causes hang and excessive cpu usage

Notes
(0016149) ohnobinki (developer) 2010-07-14 04:03	I suppose the best way to fix this is to detect and error upon any include recursion.

(0016160) syzop (administrator) 2010-07-14 16:57	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.

(0016192) warg (reporter) 2010-07-14 22:42	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.

(0016216) ohnobinki (developer) 2010-07-18 21:09	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. ;-)

(0016217) ohnobinki (developer) 2010-07-18 21:40	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.

(0016218) syzop (administrator) 2010-07-19 11:52	I see. I've rescheduled it for 3.2.10 ;)

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