[syslinux] [PATCH 2/2] core: Fix stack overflow when reloading config

[Celelibi]

2016-01-25 23:21 UTC+01:00, Ady via Syslinux <syslinux at zytor.com>:
>
>> >I see 3 ways of handling this.
>> >1) Have some specific code in ldlinux.c32 that handles
>> >reinitialization.
>> >2) Have some specific cache for the COM32 modules and load them only
>> >once for the lifetime of the whole boot loader.
>> >3) Put a file system cache that would also benefit to other files.
>> >
>> >I would tend to prefer the third way, but I don't know how much work
>> >it would be or if it would integrate well in the current design.
>> >
>> >
>> >Celelibi
>>
>> #2 is really easy; we just need to keep a copy of the unmodified
>> data section.  The only risk with this approach is running out of
>> memory, but I think that risk is minor. --
>
>
> Please forgive my lack of technical knowledge about the code itself.
>
> Regarding "...running out of memory... the risk is minor."
>
> This "wishful thinking" reminds me of a different case we had;
> although, code-wise, it was not a similar problem (I would assume).
>
> A patch from Shao, "fs: Fix searchdir resource leak", committed
> 2012Nov,
>
> _ for 5.xx commit 57acc34bdf83fc5ea08dbf44b74a5dd2c1131187
>
> _ for 4.xx commit 37971728a5fc40b1c90512e79e47333d98ec8851
>
> reads:
>  ***
> This is a significant rewrite of the generic lookup logic inside
> core/fs/fs.c's searchdir function.  Previously, there was a memory leak
> if a path involved multiple directories.  After a sufficiently large
> number of invocations, this could be observed.
>  ***
>
> The code, before that patch, used to assume that just "a few" files
> were involved. A c32 module that has to deal with many more files
> showed the leak, and the crash.
>
> (BTW, I am still waiting - after more than 3 years - and hoping for
> that useful c32 module to be part of official Syslinux.)
>
> Additionally, the amount of available resources (including memory,
> among others) in some cases is not _that_ high, and we frequently
> forget they exist. Cases such as the DOS-based installer comes to mind,
> and the length of the file names for library modules (e.g.
> "libutil_com.c32") is another one. It was _that_ easy to forget that
> FAT and ISO9660 do not support such file names (thus, a library module
> shall not use unsupported file names), and it is easy for anyone to
> assume that "his own" use-case is broad-enough that it covers anyone's
> / everyone's else.
>
> It is also not a coincidence that we already have seen leaks related to
> directives such as CONFIG and INCLUDE in the past.
>
> I hope we are not introducing more problems while we are trying to
> solve one.
>
> Regards,
> Ady.


I'll make a global reply.
First, regarding memory, we're talking about a few additional
kilobytes per module. For instance, on ldlinux.c32, the .data section
(the only one that need to be stored to be reset) is less than 300
bytes.

>From what I've seen, I think there are memory leaks a bit everywhere.
And it's practically unavoidable since modules are always forcefully
unloaded instead of letting them gracefully terminate.

Then, as I already mentioned somewhere else, I think we would need a
global resource tracker so that when a module is unloaded (or
simili-unloaded if it stays in memory anyway as discussed above) its
allocated resources (memory, opened files etc) can be freed anyway.

That would, of course, not work for the resources allocated by the
core module, but that could help tracking the growth of the allocated
resource and thus help debug them.


Celelibi