Remove the "static" modifier from the "str" variable in the odb.c/strcomb function

[Giorgio Pintaudi]

Hello, my name is Pintaudi Giorgio and I am a Ph.D. student at Yokohama National University (T2K/WAGASCI experiment).

In the last few days, I was struggling with what I suspect is a little bug of MIDAS. I think that I have found a solution to the bug and I would like you to review it.

The problem is that I was always getting a segmentation fault error when using the odb.c/strcomb function (I am developing the MIDAS interface for our experiment). The crashes never occurred the first time that the function was called, but always the n-th time where n was always the same. The error message was quite cryptic:

mremap_chunk: Assertion `((size + offset) & (GLRO (dl_pagesize) - 1)) == 0' failed. Aborted.

At first, I thought that to solve the error, it would be enough to free the memory allocated in the heap by the malloc/realloc call in the strcomb function. But that didn't work either and I got another error instead when trying to free said memory:

free(): invalid pointer

I ultimately solved the error by deleting the "static" modifier at odb.c:499

• static char *str = NULL; -> char *str = NULL;

• The advantage of this solution is, of course, to remove the error and avoid the somewhat frowned upon practice of using static variables inside of a function.

• The disadvantage is that the developer now has to be manually free the memory allocated in the strcomb function to avoid memory leaks.

If you need further info, please let me know. Best regards Giorgio

[dd1]

use of "static" in strcomb() confirmed. this is not thread-safe and has to be fixed ("static" removed, value returned by strcomb() free()ed). perhaps it will have to wait until we switch midas to c++ and change strcomb() to return std::string. looking into it. K.O.

[Stefan Ritt]

I agree with KO that the current implementation is bad since it's not thread safe. Indeed the ultimate solution will be to switch to std::string. But before we are there, I believe the best solution is to change the syntax of strcomb to

void strcomb(char **result, const char **list);

then allocate *result with the proper size and return it, having the user to free the pointer afterwards. The change of the syntax makes existing deployments not compile any more, so people are forced to change their code, read the documentation and fix it. Otherwise people might forget to free the returned points because everything compiles fine and we get tons of memory leaks that way.

Thoughts?

[Stefan Ritt]

We can also bite the bullet and return std::string. This forces also everybody to revisit their code, but it also means all experiments switch to C++.

[Giorgio Pintaudi]

I agree on both the short term fix (change of C syntax) and long-term fix (C++ std:string return).

PS maybe there is a typo in the new syntax. If the current behavior is to be preserved we should have:

void strcomb(char *result, const char **list);

[Stefan Ritt]

Nope. The result is a pointer, but it is passed as a as a function argument, so you need a pointer to a pointer. You then do something like

char *p;
strcomb(&p, &list);
... use p ...
free(p);

so you have to pass the address of p, not its value.

[Giorgio Pintaudi]

Yes, you are absolutely right. It was a blunder.

[dd1]

I am thinking in the same direction. Add strcomb1() that returns std::string. Before I do this, I want to check all users - if all users are already c++ (logger, mhttpd, etc), then the old strcomb() can be removed. K.O.

[dd1]

Added "std::string strcomb1(const char **list);". Switched all c++ code to use it. The rest of the users can switch when we switch midas to c++ soon. K.O.

[Stefan Ritt]

The C++ code looks so much nicer!

[dd1]

commit d5c7f52 provides strcomb1() that returns std::string and is thread-safe. closing this problem report. old strcomb() likely to be removed when odb.c and midas.c are switched to c++. K.O.