Bug 229888 - devel/boost-libs: Fix undefined behavior in boost::filesystem::copy
Summary: devel/boost-libs: Fix undefined behavior in boost::filesystem::copy
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: FreeBSD Office Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-19 15:07 UTC by Michael Gmelin
Modified: 2018-07-19 21:16 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (office)


Attachments
Fix to boost::filesystem::copy (1.83 KB, patch)
2018-07-19 15:07 UTC, Michael Gmelin
jbeich: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Gmelin freebsd_committer 2018-07-19 15:07:26 UTC
Created attachment 195277 [details]
Fix to boost::filesystem::copy

There is a bug in boost::filesystem::copy that leads to undefined behavior (and segfault on 11.2 + clang6 + stackprotector support).

Example:

int main()
{
  boost::system::error_code ec;
  boost::filesystem::copy("/tmp/t.pdf", "/tmp/t2.pdf", ec);
}

This is caused by derefencing null pointers.

There is already a pull request on github:

https://github.com/boostorg/filesystem/pull/71

This patch pulls in this patch (tested) and fixes the issue. The same code worked on 10.3 with clang 3.9 (at least on an older ports tree), that's why this is a ticking time bomb for people upgrading to 11.2.
Comment 1 Jan Beich freebsd_committer 2018-07-19 19:30:06 UTC
The provided example doesn't crash. Tested on 11.2 i386/amd64 with boost-libs-1.68.0 (beta1) built by clang 6.0.0 and boost-libs-1.67.0_2 built on package cluster by clang 4.0.0.

(In reply to Michael Gmelin from comment #0)
> The same code worked on 10.3 with clang 3.9 (at least on an older ports tree),

FreeBSD 10.* has Clang 3.4.1 in base while USES=compiler:c++14-lang has never used devel/llvm39 by default.

> that's why this is a ticking time bomb for people upgrading to 11.2.

Was the issue exposed by -std=gnu++14 being default since Clang 6 or can you reproduce by building the consumer with -std=gnu++98?

Otherwise, I don't understand why the issue would hit 10.3 -> 11.2 users but not 11.1 -> 11.2.
Comment 2 Michael Gmelin freebsd_committer 2018-07-19 19:43:04 UTC
(In reply to Jan Beich from comment #1)

This is because I gave you the wrong example (wasn't focused whole doing this and copy and pasted the wrong one), sorry. I came from 10.x, so not sure if 11.1 was affected as well.

This one does crash reliably here on 11.2 amd64:

int main()
{
  boost::filesystem::copy("/tmp/t.pdf", "/tmp/t2.pdf");
}

This one doesn't (the original one), as passing in ec prevents the nullptr dereference

int main()
{
  boost::system::error_code ec;
  boost::filesystem::copy("/tmp/t.pdf", "/tmp/t2.pdf", ec);
}

Background is that copy has two signatures:

void copy(const path& from, const path& to)
{detail::copy(from, to);}
void copy(const path& from, const path& to, system::error_code& ec) BOOST_NOEXCEPT 
{detail::copy(from, to, &ec);}

As you can see, the dirst version calls detail copy without ex, which means ec is 0:

void detail::copy(const path& from, const path& to, system::error_code* ec=0);

The implementation of detail::copy then calls various functions that expect an error_code reference (error_code&) by dereferencing a null pointer:

  void copy(const path& from, const path& to, system::error_code* ec)
  {
     file_status s(symlink_status(from, *ec)); // boom
     if (ec != 0 && *ec) return; // here it's checked, funny

     if(is_symlink(s))
     {
       copy_symlink(from, to, *ec); // boom
     }
     else if(is_directory(s))
     {
       copy_directory(from, to, *ec); // boom
     }
     else if(is_regular_file(s))
     {
      copy_file(from, to, fs::copy_option::fail_if_exists, *ec); // ouch
  ...

The patch replaces these calls to calls to the respective functions in the detail namespace, which all take am error_code* as input, which is allowed to be nullptr:


     file_status s(detail::symlink_status(from, ec));
     copy_symlink(from, to, ec);
     copy_directory(from, to, ec);
   ...

Hope this clarifies the issue.
Comment 3 Michael Gmelin freebsd_committer 2018-07-19 19:45:09 UTC
(In reply to Michael Gmelin from comment #2)

     file_status s(detail::symlink_status(from, ec));
     detail::copy_symlink(from, to, ec);
     detail::copy_directory(from, to, ec);

Please let me know if you need additional input, I think you should be able to reproduce it quite easily now though.
Comment 4 Jan Beich freebsd_committer 2018-07-19 20:49:35 UTC
Comment on attachment 195277 [details]
Fix to boost::filesystem::copy

Looks OK. Maybe MFH to 2018Q3 as well.
Comment 5 Jan Beich freebsd_committer 2018-07-19 20:50:12 UTC
Ah, forgot. Add the URL at the top of the patch file.
Comment 6 Jan Beich freebsd_committer 2018-07-19 20:52:16 UTC
Comment on attachment 195277 [details]
Fix to boost::filesystem::copy

> +--- libs/filesystem/src/operations.cpp.orig
> ++++ libs/filesystem/src/operations.cpp

Please, regen via "make makepatch".
Comment 7 commit-hook freebsd_committer 2018-07-19 21:05:10 UTC
A commit references this bug:

Author: grembo
Date: Thu Jul 19 21:04:45 UTC 2018
New revision: 474979
URL: https://svnweb.freebsd.org/changeset/ports/474979

Log:
  Fix runtime null pointer dereference (undefined behavior)

  PR:		229888
  Approved by:	maintainer
  Obtained from:	https://github.com/boostorg/filesystem/pull/71
  MFH:		2018Q3 (runtime fix, null pointer dereference)

Changes:
  head/devel/boost-libs/Makefile
  head/devel/boost-libs/files/patch-libs_filesystem_src_operations.cpp
Comment 8 Michael Gmelin freebsd_committer 2018-07-19 21:07:16 UTC
(In reply to Jan Beich from comment #6)

Thanks for your quick reaction and helpful advice.
Comment 9 commit-hook freebsd_committer 2018-07-19 21:16:20 UTC
A commit references this bug:

Author: grembo
Date: Thu Jul 19 21:16:03 UTC 2018
New revision: 474981
URL: https://svnweb.freebsd.org/changeset/ports/474981

Log:
  MFH: r474979

  Fix runtime null pointer dereference (undefined behavior)

  PR:		229888
  Approved by:	maintainer
  Obtained from:	https://github.com/boostorg/filesystem/pull/71

  Approved by:	ports-secteam (runtime fix blanket)

Changes:
_U  branches/2018Q3/
  branches/2018Q3/devel/boost-libs/Makefile
  branches/2018Q3/devel/boost-libs/files/patch-libs_filesystem_src_operations.cpp