Syslinux - Jan 2016 - [PATCH 2/2] core: Fix stack overflow when reloading config

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

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

On 01/26/16 07:36, Celelibi via Syslinux wrote:
> 
> 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.
> 
We actually *have* a resource tracker; the malloc() implementation is
explicitly designed so it keeps track of the owner module in order to be
able to purge the contents on module exit.  We would still do that, just
as now, the only difference is that we wouldn't purge the readonly
segment(s) of a module nor the backup copy of the read-write segment(s).

	-hpa