Re: [Lightning] Lightning frees NULL pointers, and frees others it didn´t allocate

[Paulo César Pereira de Andrade]

2014-07-26 16:10 GMT-03:00 Ian Grant <address@hidden>:

  Hi Ian,

> This one cost me eight hours of my time.

  Sorry for that, but feel free to ask or report any other issues you
find. I will do my best to make it easier to understand lightning
and better document the API.

  I made a small patch to your example, but the main issues are:

1. jit_set_memory_functions should be called before init_jit, otherwise if
   the disassembler is enabled, it (in your example my_free) will see a free
   of one or more apparently bad pointer(s), that was/were allocated in the
   internal jit_init_debug function (disasm_symbols and disasm_synthetic).
2. jit_clear_state must be called, most of the data is not useful after
   jit_emit, but it is not implicitly called to not release information used by
   jit_print (kind of high level disassembler not really showing machine asm)
   and jit_address (for different reasons one may want to know the address
   of a label after jit is actually generated). Only jit_destroy_state will not
   release all memory. Your example is not calling jit_clear_state in the
   second jit generation.

  Issue one should be documented, because it would cause problems
if the memory functions are more than plain malloc, realloc and free
wrappers.

  With the above changes, the new log finishes like this:

(after finish_jit returned)
No allocated blocks.

> Here is a program which reproduces the problem, an example of the output,
> and a patch to lightning.c which adds some debug printfs which might help
> you find the problem. I suspect it´s in jit_dataset. I have tried to fix it
> but the code there is incomprehensible to me. It seems to test flags like
> _jitc->no_data, and on the basis of this, it makes assumptions about the

  It is an internal state to prevent using a second mmap'ed block for
constants, and instead synthesise them on the stack.

> values of pointer variables. But flags like this are just another possible
> source of inconsistency. It´s almost always better to test the pointers
> directly. There are also places in this function where it calls memcpy and
> copies to a NULL pointer.

  The code assumes free(NULL) is ok, and memcpy with a 0 length is also
ok if NULL is an argument. I agree that this may be a bad practice.

> There is another problem in that the manual indicates one can use separate
> _jit states concurrently, but this is not true because jit_new_state uses a
> global variable. The result is a space leak if it´s called twice. Another

  I think you got confused and it is a side effect of your example not calling
jit_clear_state for the second run.

> problem is that init_jit keeps a pointer to the program name, but the manual
> doesn´t state this. It should either warn about this in the manual, or
> better, use strncpy to keep a private copy of the string.

  This is indeed not properly documented, and the global variable should
be removed and passed as argument to jit_init_debug. It is only used from
init_jit, that calls jit_init_debug, and instead of receiving an argument pass
it in a global variable. There isn't much reason to use a value other than
argv[0] as init_jit argument; the string will be used as argument to bfd_openr,
to try to fetch symbol names and produce better disassemble output.

  Just in case, I am attaching the patch for your example to make it
not show any leaks, and a sample new log.

> Ian

Thanks,
Paulo

patched.out
Description: chemical/gulp

testlightning.c.patch
Description: Text Data