Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Michael Matz <matz at suse dot de>]

Hi,

On Fri, 30 Nov 2018, Nick Clifton wrote:

> Not without modifying the current demangling interface.  The problem is 
> that the context structure is created for each invocation of a 
> demangling function (from outside the library), and no state is 
> preserved across demangling calls.  Thus in order to have a recursion 
> limit which is configurable by the caller, you either need to have a 
> global variable or else extend the demangling interface to include a 
> recursion limit parameter.
> 
> I did consider just having a fixed limit, that the user cannot change, 
> but I thought that this might be rejected by reviewers.  (On the grounds 
> that different limits are appropriate to different execution 
> environments). Note - enabling or disabling the recursion limit is 
> controlled by a separate feature of the proposed patch, ie the new 
> DMGL_RECURSE_LIMIT flag in the options field of the cplus_demangleXXX() 
> functions.  But there is not enough room in the options field to also 
> include a recursion limit value.

Or we decide to not ignore this part of the GNU coding standard ...

> 4.2 Writing Robust Programs
>  
> Avoid arbitrary limits on the length or number of any data structure, 
> including file names, lines, files, and symbols, by allocating all data 
> structures dynamically. In most Unix utilities, “long lines are silently 
> truncated”. This is not acceptable in a GNU utility.

... just because script kiddies do mindless fuzzing work.  I realize that 
you didn't implement a fixed limit, but IMHO it's bordering with that.

But re the static variables: you have two, once the recursion depth, and 
once the limit.

The limit can be a static variable just fine, you state, as part of the 
interface that it sets global state, and if that needs to happen from 
multiple threads that the user must synchronize.  This is fine because 
there won't be many calls to constantly change that limit.

The current depth needs to be part of the d_info (resp. workstuff) 
structure, avoiding the thread-unsafety.

Another crazy idea: do encode the limit in the option field.  If you 
declare that the limit is a power of two (which IMHO doesn't 
materially change the usefullness of such limit) then you only need to 
encode the log2 of it, for which 5 bits are plenty enough (you can encode 
limits from 1 to 1<<32 then).


Ciao,
Michael.