Issues
Closed this issue · 16 comments
Some quick initial review of libhl turned up some issues with it.
- The use of
__USE_UNIX98is a non-standard feature-test macro which should be replaced with_POSIX_C_SOURCE. - There are a lot of identifiers that begin with double underscore, for instance
__foo. These identifiers are reserved by the implementation (compiler and libc.) perhaps using anhl_prefix would be better. This is also seen in headers a lot too;__BINOMIAL_HEAP_H__for instance. - Failure to use
extern "C"in headers for__cplusplusThis makes it hard to actually use the library for C++ as the symbol names can be mangled, thus not finding them at link stage. #pragma pack(push, ...)is seen a lot. I'm not sure why you're packing these structures, for doing so can only lead to access violations on systems where the read/write may be unaligned and therefor illegal (PPC for instance.)- Checking for
NULLwhen callingfree. This is just redundant asfreeis required to do this check. Doing this only leads to false-positives when running the code through static-analysis tools like coverity. - Linked list is not thread-safe.
get_entry_positionneeds to hold a lock for a new list node can be inserted into the list in another thread causing the position to be wrong. This can cause a whole slew of problems. - Failure to check return value of
malloc. It can fail, without checking and handling this case, this library is not usable for robust systems.
ok ... some are good points ... some others not :
- I'll replace __USE_UNIX98 with __POSIX_C_SOURCE.
- Actually I used this library from c++ so what is exactly the problem you are having including it? .. it might be that some of the more recent sources haven't been wrapped with extern "C", but linklist, hashtables and queues should just work ... could you show me the output of your compiler?
I might push in some c++ wrappers I already made in past. - The #pragma pack is pretty safe and it's handled by the compiler if possible. It will be ignored if not applicable. When taken into account (which is always where I run this library) it has an effect on the size of certain structures and give some benefits in specific use cases. But since it's generally safe I wouldn't bother.
- The extra checks before calling free could be indeed removed nowadays .... consider that code (linkedlist) comes from old times before being imported in libhl , I'm pretty sure I've added those checks because on some old libc the free() implementation wasn't checking for the pointer being NULL.
- I'll fix get_entry_position if that's the case .... let me know if you already have a patch to apply
- Wrapping malloc calls doesn't make any sense to me since if you run out of memory you want your program to die. If you think it would add robustness let me see a patch and I'll be glad to review it
But in general failing gracefully if out of memory is not an option, since the whole system fails in this case and your process would probably be killed. Consider this library has not been developed to run
on embedded systems but on critical services running on unix systems.
You could let the library fail gracefully and let the outer code raise an exception ... but still, if you think this is something important please submit a patch. - The use of '__' is completely legit and actually is a sort of standard (did you ever grep for "define __" in /usr/include/** ?)
-
I suggest to avoid ignorance of return values a bit more.
Are you interested to apply aspect-oriented software development?
How do you think about to encapsulate error detection and corresponding exception handling as a reusable aspect in your software? -
How do you think about to avoid that this software depends on undefined behaviour?
could you point out where return values are ignored please? ... or where does the software depend on undefined behaviour?
(or better push a patch if you think there is something wrong)
How do you think about care also for the return value from the call of a function like "pthread_mutex_init"?
Would you like to improve static source code analysis also for your software?
Do you want that these source files should only be safely reused within the implementations of compilers for the programming languages "C" and "C++"?
ok ... added some extra checks to fail gracefully when out of memory conditions happen.
Also mutex and attrs initialization is checked ... if you have any hint on how to improve the static analyzer you are more than welcome , so far though I didn't see the clang static analyzer having any problem.
I can't understand your last comment.
Thanks for your time and let me know if you see anything else worth improving :)
... if you have any hint on how to improve the static analyzer ...
How do you think about to try any more source code analysis tools out?
Are there a few update candidates left over?
I can't understand your last comment.
Would you like to avoid to tamper with identifiers in the reserved name space more than it is absolutely necessary?
are you sure that the "a few update candidates" link you posted points to the right place?
also, about tampering with the reserved name space, do you refer to the preprocessor symbols on top of the header files?
maybe I could use a single underscore instead of two ? but did you have any name clash so far?
did you check if it's common practice to use such identifiers (<UPPERCASE_FILENAME>_H) on top of C header files?
(I don't know if I like this game of talking only by questions .... anyway .... I'll close this issue, if you see any problem please start a new issue and attach some useful information to fix or improve the code instead of playing the riddle game)
are you sure that the "a few update candidates" link you posted points to the right place?
Yes. - My link pointed to more functions from the POSIX thread programming interface for further considerations.
also, about tampering with the reserved name space, ...
@graphitemaster pointed similar concerns out for the identifier selection. It will be safer to replace some underscores by an unique prefix or delete them.
..., if you see any problem please start a new issue ...
The software improvement can be continued with bug reports like the following.
The use of '__' is completely legit and actually is a sort of standard (did you ever grep for "define __" in /usr/include/** ?)
Files in /usr/include are the files provided by the implementation. They are allowed to use the double underscore. You as a library cannot however as the standard explicitly states that anything beginning with double underscore is reserved by the implementation. It's legal for stdio.h for instance to expose a macro like #define __foo int If your code now uses this identifier, bad things will happen.
Check 7.1.3 Reserved identifiers in the C99 specification which states
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
I did not suggest to wrap malloc calls. I suggested checking their return status to see if they return NULL and to gracefully propagate this error back up the call stack and indicate error to the user of the library. A library that cannot indicate failure of memory allocation is not usable in a robust environment where having a service just unexpectedly crash because of OOM just isn't allowed. Flight control software for instance.
Have you checked the changes from yesterday?
I think the library now properly handles OOM conditions and return back an error to the caller.
Now, I can understand the point if you are going to use this library in some realtime application but if you run on a generic unix system and you run out of memory chances are that your process will be killed by the system or that your program will just crash next time you call any function from the standard library or not (consider that apart a few functions which return back an error when out of memory, many other will just crash).
Now, I'm not saying that you shouldn't care about such out-of-memory cases, but I think you should handle certain conditions only in contexts where it makes sense
(for instance advising to always check the return value of pthread_mutex_lock() doesn't make any sense in 99% of contexts).
I understand that such a library is pretty generic so I shouldn't have assumed anything and just let the caller make the decision ... hence the recent improvements thanks to your suggestions.
But next time a simple patch instead of a series of allusive comments and references to guidelines and conventions would be better and more appreciated.
In the end if you don't like this library on you don't trust the code, you can just not use it ;)
If instead you are going to use it, please try being more constructive in the feedbacks and provide some code.
... for instance advising to always check the return value of pthread_mutex_lock() doesn't make any sense in 99% of contexts ...
I recommend to prepare the source code of this basic data structure library also for the consistent handling of exceptional situations.
But next time a simple patch instead of a series of allusive comments and references to guidelines and conventions would be better and more appreciated.
It is occasionally more convenient when contributions are provided in the source code language directly. But software evolution will need some consensus before proposed changes will be accepted.
There are further design options available to choose the preferred solution.
I understand that patches are more helpful than a series of elusive comments. However some of us are prevented from providing actual working source code due to non compete clauses preventing us from achieving that. Consider feedback the only choice in that circumstance.
@graphitemaster if you're using this in a commercial project, it might be nice to provide @xant some contact work to fix these issues.
Non competition caluses and non disclosure agreements are pretty common in the software development industry. But unless your business is about building libraries for basic datastructures and standard algorithms, I don't see how contributing with fixes/improvements to such an opensource component might fall into those caluses.
And in general for any kind of business, if you end up employing some opensource component, it would be nice to contribute back with actual patches or at least with a clear and explanatory feedback.
Also being nice and polite would help, even if this is not strictly necessary.