[thread] age-old, fatal, trivial-to-fix bug

(Sorry for the flashy topic, but I reported the bug already ages ago and I'm slightly annoyed that it is still present. But hey, at least I could restrain myself from using multiple exclamation marks.) Hi! If you compile libs/thread/src/exception.cpp, some compilers will warn you about an unused variable (the argument to system_message()) when not compiling for win32. Firstly, the fact that it doesn't use this variable but uses errno directly to format a string is already broken, after all I explicitly requested it to format a string for the given errorcode, not one that happens to be in errno at that time (well, you could argue that that is intended, after all the function is undocumented, but then you'd have to do the same with win32 and use GetLastError() there). Secondly, look at the way that function is used. It is used in thread_exception::message() to format a std::string temporary and then return the result of c_str() from a function! This returns a reference to a temporary and the returned pointer can never be used! It can't work, simple as that, it's broken and as far as I'm concerned it is even obviously so. Solution: remove thread_exception::message(), no sane code could ever have depended on that function anyway. Remove system_message() all along, it is unused then. There is a slew of other code that is intended to access strerror() (for POSIX) or FormatMessage() (for win32) in a portable way, which is then also unused (of course, after all system_message() is a wrapper around those and with it unused, those can go, too). Please, please, please fix this! I have attached the patch for current CVS but the same is already present in at least 1.33 and probably older versions, too. thank you Uli

Ulrich Eckhardt wrote:
(Sorry for the flashy topic, but I reported the bug already ages ago and I'm slightly annoyed that it is still present. But hey, at least I could restrain myself from using multiple exclamation marks.)
If I remember well this topic already has been answered. What you are reporting is not a bug, but a cosmetic issue. The code in question will never be called. I intend to fix this in the next release, since some rewrite will be necessary to correctly return system messages. It is too short before release to introduce new behaviour. Please correct me if I am wrong in my assumption, that the reported behavior will not affect a user of the library. Thank you for your kind understanding. Roland

On Saturday 10 February 2007 12:24, Roland Schwarz wrote:
Ulrich Eckhardt wrote:
(Sorry for the flashy topic, but I reported the bug already ages ago and I'm slightly annoyed that it is still present. But hey, at least I could restrain myself from using multiple exclamation marks.)
If I remember well this topic already has been answered. What you are reporting is not a bug, but a cosmetic issue.
The code is simply broken, and dangerously so. I wouldn't call it cosmetic.
The code in question will never be called.
The broken function is declared in a public header, why shouldn't some user call it? Also, I know a way to make sure that it isn't called...
I intend to fix this in the next release, since some rewrite will be necessary to correctly return system messages. It is too short before release to introduce new behaviour.
Ah, wait: the patch (probably) applies to older versions, but it was taken against the CVS which reports version 1.35!
Please correct me if I am wrong in my assumption, that the reported behavior will not affect a user of the library.
Unnecessary bloat. Maybe, when someone calls the code, crashes. I'm a bit frustrated, please don't take it as a personal attack. Uli

Ulrich Eckhardt wrote:
Unnecessary bloat. Maybe, when someone calls the code, crashes.
Bloat yes, crash no. None can call it in a way, it would crash. Since the m_sys_err cannot be non-zero. Only if a user code calls throw lock_error(...) , which user code isn't expected to, then trying to access the (undocumented internal) function message this will ever be possible.
I'm a bit frustrated, please don't take it as a personal attack.
No need to. The patch finally is applied. Some words though. 1) If you had pointed me to the old posting where I could have had seen, that the previous maintainer (Michael Glassford) already had decided to apply this patch, I would have had saved time, and just applied. 2) You, and every other have the right to expect from me, that I will not blindly apply patches without review. (And this always costs time, which I not always have.) 3) I want to thank you for your patience, and invite you to further contributions. Roland
participants (2)
-
Roland Schwarz
-
Ulrich Eckhardt