POV-Ray

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.

Tasklist

FS#213 - Missing check for ios failbit/badbit: endless loop while trying to read a directory as config file

Attached to Project: POV-Ray
Opened by Jan-Philip Gehrcke (jgehrcke) - Wednesday, 22 June 2011, 13:50 GMT
Last edited by Christoph Lipka (clipka) - Tuesday, 17 July 2012, 20:45 GMT
Task Type Definite Bug
Category Backend → Parser/SDL
Status Closed
Assigned To Jim Holsenback (jholsenback)
Operating System Linux
Severity Medium
Priority Normal
Reported Version 3.70 RC3
Due in Version 3.70 RC4
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

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

This task depends upon

Closed by  Christoph Lipka (clipka)
Tuesday, 17 July 2012, 20:45 GMT
Reason for closing:  Fixed
Comment by Jan-Philip Gehrcke (jgehrcke) - Wednesday, 22 June 2011, 13:53 GMT

I should have diffed the other way round... anyway, you get what I mean :D

JP

Comment by Thorsten Fröhlich (thorsten) - Thursday, 23 June 2011, 07:09 GMT

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 :-)

Comment by Jim Holsenback (jholsenback) - Thursday, 23 June 2011, 07:30 GMT

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?

Comment by Christoph Lipka (clipka) - Thursday, 23 June 2011, 17:22 GMT

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.)

Comment by Jim Holsenback (jholsenback) - Friday, 24 June 2011, 09:54 GMT

Did some testing before submitting this fix:

if (!Stream)
{
	fprintf(stderr,
		"%s: %s: %lu: invalid filename specified '%s' for %s: ",
		PACKAGE, conf_name.c_str(), line_number, line.c_str(), sections[section].label
	);
	break;
}

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.

Comment by Jim Holsenback (jholsenback) - Friday, 24 June 2011, 11:18 GMT

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):

			if (!Stream)
			{
				fprintf(stderr, "%s: cannot open the user configuration file ", PACKAGE);
				perror(m_userconf.c_str());				
				break;
			}

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.

Comment by Jim Holsenback (jholsenback) - Friday, 24 June 2011, 14:11 GMT

change #5455 submitted

Comment by Jim Holsenback (jholsenback) - Friday, 24 June 2011, 16:31 GMT

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.

Comment by Jim Holsenback (jholsenback) - Friday, 24 June 2011, 18:48 GMT

OK ... change #5456 seems to work as expected.

			// test to avoid endless loop condition
			if ( Stream.bad() )
			{
				fprintf(stderr, "%s: cannot open the user configuration file ", PACKAGE);
				perror(m_userconf.c_str());
				break;
			}

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

Comment by Jan-Philip Gehrcke (jgehrcke) - Saturday, 25 June 2011, 00:36 GMT

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:

  • While doing IO on a stream in a loop, before or after each IO operation, all possible error bits have to be checked before invoking any operation on the data gained. Other solutions are a clear no go and must not be considered. With change #5456 the failbit check is omitted.
  • The error string in your change is not correct for the following reasons:
  1. "cannot open": we cannot limit this to only opening errors. Errors can also occur after having read some data successfully.
  2. "user configuration file": Too specific, because this function is also called for the system configuration file.
  3. "m_userconf.c_str()": a wrong file name may be printed here; confer (b) (easy to fix)

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:

  • badbit and failbit are not made to tell e.g. if opening a file failed. They just tell you that the stream is not ready for IO operations anymore.
  • Stream.is_open() obviously is not able to catch the is-directory-case. To catch this properly, Boost.Filesystem would have to be used.
  • perror() evaluates errno and it may depend on the system's implementation if C++'s ios functions set errno to meaningful values (although this should work for unix).
  • perror() always prints the last error, which may have been set 20 lines before and I am not sure if e.g. istream::getline sets errno correctly. Maybe this led to confusion during your tests. Hence, in case of badbit/failbit, we could provide our own unspecific error message and forget about perror().

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 :)

Comment by Jim Holsenback (jholsenback) - Saturday, 25 June 2011, 09:58 GMT

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.

Comment by Jim Holsenback (jholsenback) - Saturday, 25 June 2011, 13:11 GMT

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:

if ( Stream.bad() )
			{
				fprintf(stderr, "%s: cannot open or read configuration file %s\n", PACKAGE, conf_name.c_str() );
				break;
			}

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.

Comment by Jan-Philip Gehrcke (jgehrcke) - Saturday, 25 June 2011, 20:43 GMT

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:

        while(1)
        {
            // try to get line from stream
            std::getline(Stream, line);
            // check for failbit and badbit and eofbit
            if(!Stream.good())
            {
                // Only in case of the badbit we can assume that some lower
                // level system API function has set errno and perror() can be
                // used safely.
                if(Stream.bad())
                {
                    fprintf(stderr, "%s: error while reading/opening configuration file ", PACKAGE);
                    perror(conf_name.c_str());
                }
                break;
            }
            // preprocess line
            line = pre_process_conf_line(line);

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

Comment by Jim Holsenback (jholsenback) - Sunday, 26 June 2011, 10:28 GMT

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.

Comment by Jan-Philip Gehrcke (jgehrcke) - Sunday, 26 June 2011, 12:25 GMT

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

Comment by Jan-Philip Gehrcke (jgehrcke) - Wednesday, 06 July 2011, 13:50 GMT

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):

        // Read file loop start:
        // Try to iteratively get line from Stream
        // Abort in case of badbit or failbit
        while(std::getline(Stream, line))
        {
            [...]
        }  // Read file loop end
        // Only in case of the badbit we can assume that some lower
        // level system API function has set errno. Then, perror() can be
        // used safely.
        if(Stream.bad())
        {
            fprintf(stderr, "%s: error while reading/opening config file ", PACKAGE);
            perror(conf_name.c_str());
        }

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).

Comment by Jim Holsenback (jholsenback) - Thursday, 07 July 2011, 16:22 GMT

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.

Loading...