The Persistence of Vision Raytracer (POV-Ray).
This is the legacy Bug Tracking System for the POV-Ray project. Bugs listed here are being migrated to our github issue tracker. Please refer to that for new reports or updates to existing ones on this system.
Opened by Jan-Philip Gehrcke - 2011-06-22
Last edited by Christoph Lipka - 2012-07-17
FS#213 - Missing check for ios failbit/badbit: endless loop while trying to read a directory as config file
Hello,
this bug report relates to this thread: http://news.povray.org/povray.beta-test/thread/%3Cweb.4e0064d9a3970d212b256d410%40news.povray.org%3E/
In /unix/unixoptions.cpp around line 817:
while ( !Stream.eof() ) { // get and preprocess line std::getline(Stream, line); line = pre_process_conf_line(line); ++line_number; // skip empty line if(line.length() == 0) continue; [...] }
`Stream.eof()` could never be true as well as `line.length()` always can be zero, leading to an endless loop. In my case, the problem occurred due to my ~/.povray/3.7/povray.conf being a directory instead of a file.
To fix the problem, one has to consolidate this loop by not only checking for eofbit, but also for failbit and badbit of `Stream`. The getline() method on a directory does not set the eofbit, but it sets fail and bad (test: http://codepad.org/RiZGo3ia). According to http://www.cplusplus.com/reference/iostream/ios/operatornot/ fail and bad can be checked for by `!Stream`. A simple `if (!Stream) break;` within the loop fixes the problem (patch file attached).
Are there more missing checks on iostreams like this in the code? :) They should definitely be fixed.
Furthermore, one could think about using the boost filesystem library to be able to distinguish files and directories to provide good error messages.
Hope that helps,
Jan-Philip Gehrcke
I should have diffed the other way round... anyway, you get what I mean :D
JP
Are there more missing checks on iostreams like this in the code? :) They should definitely be fixed.
They should be fixed indeed, but it should be easy to check as the core code does not use standard iostreams (too slow implementations on some systems). This leaves only the Unix frontend code to check, which is a fairly small part of the code
I see that !Stream.eof() only appears the one time in vfe/unix/unixoptions.cpp (other files in that directory also checked). The users proposed fix looks sane enough to check in a change ... agreed?
Checked for other occurrences of this problem, too, but haven't found anything in the code either, so the proposed fix should be sufficient... almost. I suggest spitting out a suitable error message in addition. (Note that read fails might theoretically occur anywhere in the file, so a message saying "%s seems to be a directory" may not be very fitting.)
Did some testing before submitting this fix:
with unexpected results. after renaming my users povray.conf file and creating a directory 'povray.conf' in it's place, make check ran the biscuit scene file just fine but produced two full screen images (I'm dual monitors) and locked up my system (front panel reset was my only option) as I had no keyboard or mouse response. I suspect libsdl enabled x11 as the culprit.
Once I regained my system (and before running make install) a personal test file reproduced the runaway loop with my povray.conf as a directory. Indeed kill -9 got me the command line again. I then ran make install and reran my personal test file. The trap indeed DOES work and produced this message to stderr:
povray: /usr/local/etc/povray/3.7/povray.conf: 96: invalid filename specified
for [Permitted Paths]: povray: /home/ash/.povray/3.7/povray.conf: 0: invalid filename specified
for : Persistence of Vision™ Ray Tracer Version 3.7.0.RC3 (g++ 4.4 @i686-pc-linux-gnu)
it appears just after this opening message (but before other credits):
povray: This is a RELEASE CANDIDATE version of POV-Ray. General distribution is discouraged.
OK so just ignore the error message (it's just an loosely modified clone of a construct lifted from code later on in the same source file, I was just getting my bearings) ... I think the point here is that I expected a bail of the render process after the error report but the file rendered anyway. The error message ends up being obscured by the other text credits etc (I didn't notice it until I scrolled up) .... is there a way to signal a bail on the render process so the user knows that there is a potential problem that should be addressed?
Like I said the trap works, but it seems like we should do better with the /error/ handling ... maybe this should be classified as a warning because the only time the end user would see a problem is if they source a scene file that's not in the standard path ... then they would get a bail on the render. I think the trap should throw them back to the command line (abort render) and make the error message a bit more obvious that something needs to be addressed before continuing.
Sorry for the extra bandwidth, but I found a way to report this error consistent with the way other errors are being handled. This code (lifted from read the user configuration file):
produces these errors:
povray: cannot open the user configuration file /home/ash/.povray/3.7/povray.conf: Invalid argument
povray: cannot open the user configuration file /home/ash/.povray/3.7/povray.conf: Is a directory
If there are no objections I will summit that as the fix when I get more time.
change #5455 submitted
hmmm ... did some more post fix testing and discovered than when I put everything back right by removing the directory ~ash/.povray/3.7/povray.conf and putting my legit povray.conf file back in place, I still get these two error messages:
povray: cannot open the user configuration file /home/ash/.povray/3.7/povray.conf: Invalid argument
povray: cannot open the user configuration file /home/ash/.povray/3.7/povray.conf: Invalid argument
When I remove the error reporting and recompile, it appears that there is a need for some error flag to be set, because when I run the test again with povray.conf as a directory, the trap breaks it out of the loop just fine, but reporting that something is not correct with povray.conf is lost.
For sure it's /still/ getting caught by the test and issuing the above mentioned messages even when things are in order, so I'm not so sure that this is the correct place to trap ... or maybe it's just my choice for the "perror" argument isn't correct. Some additional guidance would be much appreciated, if I'm to resolve this.
OK ... change #5456 seems to work as expected.
is what ended up working ... took a little bit of research but I nailed it once I found a list of ios member functions. BTW: Stream.fail /didn't/ give the correct results
Jim,
just a quick comment from my side.
The two most imporant points
1) In your comments it was difficult for me to extract the precise test condition/result relations, so I am not really sure which exact problems you encountered under which exact circumstances.
2) The change you proposed or maybe already checked in is a wrong solution for the following reasons:
How to solve this
Regarding the loop, we have to distinguish between consolidating it and printing meaningful error messages.
For making it stable under all circumstances, there is no way around using either (Stream.eof() and !Stream) or Stream.good() or (Stream.bad() and Stream.eof() and Stream.fail()) – in other words: we have to check all three error bits. This is not difficult and for sure will not introduce unexpected behavior – it will only resolve it.
Printing meaningful error messages is a bigger issue. The reasons are:
Conclusion
I would – as before – suggest to check for Stream.eof() and !Stream (fail and bad) separately. In the latter case (fail or bad), we should provide an error message like "could not read configuration from %filename%", because we cannot be more specific without other tools.
Maybe I will manage to propose and properly test another patch during the next days addressing these problems.
Hope that helps and good night :)
Yes my comments do tend to ramble ... sorry about that
I'd considered some sort of "OR" tests in the trap and that's probably the way to go
Here's a summary without the fluff:
* !Stream produced the error message even when things were correct
* Stream.is_open produced compile error (not a Stream member function)
* Stream.fail didn't catch the error
* Stream.bad is was the only test that appeared to work
And yes about the error message it is rather specific to a particular problem. I will explore making this fix a bit more robust ... this has become a challenge of sorts and I'd like to see it through to completion.
I was able to do a bit of testing before starting my day and this setup is the /ONLY/ configuration I could get to yield the desired results:
these combinations in the trap issued the error message /even/ when things were as they should be:
if ( Stream.bad() || Stream.fail )
if ( Stream.bad() || Stream.eof )
if ( Stream.bad() || Stream.fail || Stream.eof )
I could not find any reference to %filename% that you mentioned in your previous comments, so I did a little digging and found that the conf_name.c_str function properly identified the file in question.
One other case I tested (besides the users povray.conf being a directory) was if the users povray.conf was missing which was caught by another trap later on and identified with the following message:
povray: cannot open the user configuration file /home/ash/.povray/3.7/povray.conf: No such file or directory
I also tested an /empty/ user povray.conf file just by doing touch povray.conf that however never made it into the trap and issued /no/ indication that anything was wrong ... which was kind of what I expected.
I'm anxious to see if you can come up with something better, as my interest is of course to have the problem fixed, but also to learn a bit about this process, so thanks for your patience.
Hey Jim !
I used %filename% just as a general placeholder, because I did not remember the variable name and already closed all the source files... you've found what I meant: conf_name.
Okay, prepare for reading a lot, I am sorry. First of all, after my readings and tests, I've written a blog post:
http://gehrcke.de/2011/06/reading-files-in-c-using-ifstream-dealing-correctly-with-badbit-failbit-eofbit-and-perror/
Explanation of your confusing test results
Based on the knowledge provided in the blog post, we can explain why you got wrong error messages:
Imagine you try to open a file that does not exist (e.g. the system configuration file). This sets `errno` permanently until something else sets `errno` to a new value. Then, you open a file with `ifstream s ("file")` that this time _does_ exist (say, the user config file in this case) and has content. Keep in mind: no change of `errno`, it is still set to the error from before.
You then use `getline()` in a loop. Note, that `getline()` does not set/change `errno` except for the directory-case! _Directly_ after each `getline()` call you do `if (!s) {perror(); break;}`. The file is valid and has content, so you do not enter this trap. You read until the end of file, so the last `getline()` call sets the eofbit. But we have also learned, that it may also set the failbit in this case. Everything was fine up to here, the file was read correctly until the end and the content was evaluated. The error state of the stream now is/can be that eofbit and failbit are set. One more check of `!s` left: you evaluate badbit and failbit in the last iteration before breaking out of the loop. And here it happens. You do not simply break out of the loop, you also call `perror()`! Nothing inbetween has changed errno, because `getline()` did not change `errno`. So your call to `perror()` tells the user "no such file or directory" – an error from a thousand years ago, valid for the system configuration file, but not valid anymore the current context. And even worse, you mix this error state up with the filename of the file that has actually just been read out successfully. Puh.. it took me some time, too .
Patch proposal
This is implementing the "ideal solution including error messages" from my blog post:
I tested this with respect to the ~/.povray/3.7/povray.conf file and it correctly gives me "no such file or directory" and "is a directory" errors in the corresponding test cases.
JP
JP,
I spent last evening going over your blog write up and submitted change #5457 this morning. I certainly did not fully understand the process, but I do now! Thanks for the "teaching" moment ... and of course helping to fix the problem.
Thanks for your words, Jim (also on the blog) :). I hope you've tested the patch thoroughly, because I only did it quite quickly.
Cheers,
Jan-Philip
It's me again. We have not been perfect A reader of my blog (link above) has left a comment indicating another problem with the current solution: If the last line of the file read has no trailing newline, it is not processed.
Each line should have a trailing newline character, but sometimes people edit their files with ugly editors and we definitely have to take care of that. The solution to this problem is fairly simple (do not use eofbit as criterion to not process data):
If you want to know details, I revised the blog post mentioned above. In addition to the snippet above, I also attach a patch file (converted all tabs to 4 spaces before doing the diff via :retab in vi, because I did not want to change my vi config).
JP,
The second part of the fix (later on in the same file) explains away some early on behavior that I was seeing while testing. I think getting past the idea that the malformed EOF case was an error is what got everything back on track. I'll be sure to put this through enough tests to verify it captures and reports everything properly.
Thanks for the follow up.