New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

directory_iterator_construct: Avoid provoking undefined behaviour #77

Merged
merged 1 commit into from Sep 6, 2018

Conversation

Projects
None yet
4 participants
@mikecrowe

mikecrowe commented Jun 29, 2018

The directory_iterator(const path& p) constructor calls
detail::directory_iterator_construct passing nullptr (as 0) for the ec
parameter.

detail::directory_iterator_construct may call
directory_iterator::increment(*ec) which dereferences the nullptr provoking
undefined behaviour.

On GCC, and probably many other compilers, it seems that this merely causes
a null reference to be passed to directory_iterator::increment which in
turn, turns it back into nullptr when calling
detail::directory_iterator_increment which knows how to deal with the
nullptr and is happy.

Unfortunately, directory_iterator::increment(system::error_code& ec) is
marked noexcept (unlike the version that takes no arguments) but
detail::directory_iterator_increment throws exceptions when ec == nullptr.
This results in std::terminate being called.

The simplest way to fix this is for detail::directory_iterator_construct to
just pass on the potentially-nullptr ec argument to
detail::directory_iterator_increment rather than going via the
potentially-noexcept directory_iterator::increment member function.

(Discovered in the field with Boost 1.63 and a faulty disk. I managed to
reproduce the problem by teaching dir_itr_increment to pretend that
readdir_r_simulator returned an error when it saw a certain filename.)

directory_iterator_construct: Avoid provoking undefined behaviour
The directory_iterator(const path& p) constructor calls
detail::directory_iterator_construct passing nullptr (as 0) for the ec
parameter.

detail::directory_iterator_construct may call
directory_iterator::increment(*ec) which dereferences the nullptr provoking
undefined behaviour.

On GCC, and probably many other compilers, it seems that this merely causes
a null reference to be passed to directory_iterator::increment which in
turn, turns it back into nullptr when calling
detail::directory_iterator_increment which knows how to deal with the
nullptr and is happy.

Unfortunately, directory_iterator::increment(system::error_code& ec) is
marked noexcept (unlike the version that takes no arguments) but
detail::directory_iterator_increment throws exceptions when ec == nullptr.
This results in std::terminate being called.

The simplest way to fix this is for detail::directory_iterator_construct to
just pass on the potentially-nullptr ec argument to
detail::directory_iterator_increment rather than going via the
potentially-noexcept directory_iterator::increment member function.

(Discovered in the field with Boost 1.63 and a faulty disk. I managed to
reproduce the problem by teaching dir_itr_increment to pretend that
readdir_r_simulator returned an error when it saw a certain filename.)
@mikecrowe

This comment has been minimized.

mikecrowe commented Jun 29, 2018

I've reproduced the same problem and confirmed the fix with boost 1.66 too. Unfortunately I was unable to get the current state of Boost master to compile in order to run the test suite there. Hopefully travis/appveyor will tell me if I've broken anything.

@mikecrowe

This comment has been minimized.

mikecrowe commented Jul 2, 2018

The AppVeyor test failure appears to be:

libs\filesystem\test\operations_test.cpp(1394): test 'fs::exists(link)' failed in function 'void __cdecl `anonymous-namespace'::remove_symlink_tests(void)'

The remove_symlink_tests test doesn't even seem to use directory_iterator, so it looks like this failure may not be related to my patch.

@mikecrowe

This comment has been minimized.

mikecrowe commented Aug 9, 2018

Is there anything else that I need to do? Is my explanation not clear?

@pdimov pdimov merged commit b8d259f into boostorg:develop Sep 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@b-spencer

This comment has been minimized.

b-spencer commented Sep 6, 2018

This fixes the second issue of #53, fixed by aa2a58a. But, the first issue in #53, fixed by 1303b3e, still seems outstanding. I will look at rebasing #53.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment