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>:
> This one cost me eight hours of my time.

  I pushed 3 commits that should address the issues you noticed:

http://git.savannah.gnu.org/cgit/lightning.git/commit/?id=0d96d24073bbe42054d7de6a7739b9db07d54e96
o Adds a note needing to call jit_set_memory_functions before init_jit

http://git.savannah.gnu.org/cgit/lightning.git/commit/?id=a8c180a926638ba6a4967b2a60dbea994c1f15cc
o Remove the jit_progname global, but not advertised, variable

http://git.savannah.gnu.org/cgit/lightning.git/commit/?id=0d9ac79a128524a57b951dc90f928e56531632b7
o Do not pass NULL as argument to free (the wrapper checks for NULL),
   memcpy and memmove (new wrappers checks for 0 size argument).

> 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

  I also added a comment about something that could be misleading
when reading the code, as in your patch:

-    _jit->note.length = 0;
+        _jit->note.length = 0; // if there was no_note, would you
expect there to be length and size?

and it was actually initialized to 1, because the first entry is always
allocated, but it may need to be reverted if after jit_init(), the user
tells lightning to not generate annotations; those are really only useful
when debugging code generation, as the "builtin" disassembler can
make debugging a lot easier than with a plain (gdb) x/<num>i, e.g.
one can add notes with jit_note() for something other than mapping
source code to native code, but to also just add code markers when
debugging the code calling jit_* macros/functions.

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

Thanks,
Paulo