Moved out of memory prototype to error.h, as the definition is in error.c

Signed-off-by Adriaan de Jong <[email protected]...>

---
 buffer.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

buffer.c +0 -0

index 2f8e4b8..c39bbcb 100644

Hi,

Quoted 10 lines · Adriaan de Jong

Signed-off-by Adriaan de Jong<[email protected]...>

---
  buffer.c |   29 ++++++++++-------------------
  1 files changed, 10 insertions(+), 19 deletions(-)

buffer.c +0 -0

index 2f8e4b8..c39bbcb 100644

buffer.c gc_malloc (size_t size, bool clear, struct gc_arena *a) · line 315

Quoted 6 lines

buffer.c +0 -3

@@ -310,28 +310,19 @@ gc_malloc (size_t size, bool clear, struct gc_arena *a)
  #endif
  {
    void *ret;
-  if (a)
-    {
-      struct gc_entry *e;

NACK on that. We discovered that there are use cases where "second-level" callers pass NULL, seemingly on purpose to allocate global memory instead of "in the GC".

main(), in openvpn.c:

           /* initialize environmental variable store */
           c.es = env_set_create (NULL);

... which calls into misc.c:

struct env_set *
env_set_create (struct gc_arena *gc)

{
   struct env_set *es;
   ALLOC_OBJ_CLEAR_GC (es, struct env_set, gc);
   es->list = NULL;
   es->gc = gc;
   return es;
}

... and boom, assert() fail.

Adriaan saw the crash ("my nice OpenBSD test run!!!") and promised to work on this on the way back in the train.

Quoted 42 lines · Gert Doering

Hi,

Quoted 10 lines · Adriaan de Jong

Signed-off-by Adriaan de Jong<[email protected]...>

---
  buffer.c |   29 ++++++++++-------------------
  1 files changed, 10 insertions(+), 19 deletions(-)

buffer.c +0 -0

index 2f8e4b8..c39bbcb 100644

buffer.c gc_malloc (size_t size, bool clear, struct gc_arena *a) · line 313

Quoted 7 lines

buffer.c +0 -3

@@ -310,28 +310,19 @@ gc_malloc (size_t size, bool clear, struct gc_arena *a)
  #endif
  {
    void *ret;
-  if (a)
-    {
-      struct gc_entry *e;
 

NACK on that. We discovered that there are use cases where "second-level" callers pass NULL, seemingly on purpose to allocate global memory instead of "in the GC".

main(), in openvpn.c:

           /* initialize environmental variable store */
           c.es = env_set_create (NULL);

... which calls into misc.c:

struct env_set *
env_set_create (struct gc_arena *gc)

{
   struct env_set *es;
   ALLOC_OBJ_CLEAR_GC (es, struct env_set, gc);
   es->list = NULL;
   es->gc = gc;
   return es;
}

Quoted 6 lines

... and boom, assert() fail.

Adriaan saw the crash ("my nice OpenBSD test run!!!") and promised to work on this on the way back in the train.

But should this actually call gc_malloc() in this case? gc_malloc is for things which are supposed to be tackled by the garbage collector.

Things aren't too easy here - but I'd rather see us cleaning up this instead of making gc_malloc() vague in its purpose.

$ git grep env_set_create
init.c:  c->c2.es = env_set_create (NULL);
manage.c:  struct env_set *es = env_set_create (&gc);
misc.c:env_set_create (struct gc_arena *gc)
misc.h:struct env_set *env_set_create (struct gc_arena *gc);
multi.c:    es = env_set_create (&gc);
openvpn.c:        c.es = env_set_create (NULL);

There are two places where env_set_create() is called with NULL. Let's rather see if we can provide a gc_arena pointer those places instead. Or do something which is more clear in regards to memory allocation in env_set_create().

-----Original Message-----
From: Jan Just Keijser [mailto:[email protected]...]

I agree with Gert:
I spent most of my train journey yesterday figuring out what the next assert failure/segfault was , caused by this buffer.c change; env_set_create is one, log_history_add was also giving me problems, etc etc.
With the 2.2.2 version of gc_malloc my Elliptic Curve patch works like a charm (in the git version of OpenVPN).

The idea behind this change is good, but it needs a LOT of testing before we can proceed.

Signed-off-by Adriaan de Jong<[email protected]...>

---
  buffer.h |    4 +---
  error.h  |    3 +++
  2 files changed, 4 insertions(+), 3 deletions(-)