Bug 1539386

Summary: yaml-cpp 0.5.3 breaks existing binaries compiled against older versions
Product: [Fedora] Fedora EPEL Reporter: Avi Kivity <avi.kivity>
Component: yaml-cppAssignee: Richard Shaw <hobbes1069>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: unspecified    
Version: epel7CC: guido.grazioli, hobbes1069, kodtaku+work
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: yaml-cpp-0.5.1-1.el7.2 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-01-31 19:52:24 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Avi Kivity 2018-01-28 13:43:34 UTC
Description of problem:


"yum update yaml-cpp" will break applications compiled against yaml-cpp-0.5.1. See for example https://github.com/scylladb/scylla/issues/3161 and https://github.com/scylladb/scylla/issues/3157.

Version-Release number of selected component (if applicable):
yaml-cpp-0.5.3-7.el7.x86_64

How reproducible:
Always

Steps to Reproduce:
1. Build an application against yaml-cpp-0.5.1
2. Update to yaml-cpp-0.5.3
3. Run the application

Actual results:
Crash, memory corruption, world ends.

Expected results:
Upgrade has no effect except for bugs fixed.

Additional info:


yaml-cpp is a header and template intensive library. Any small change in a header can cause upgrades to fail. IMO, the best way to fix it is to remove the shared library and replace it with a static library, so that the application and library must be updated in lock step.

Alternatively, keep the shared library, but add a static library. Applications will still break, but when they find out, they'll be able to switch to the static library.

Comment 1 Richard Shaw 2018-01-28 14:12:51 UTC
I really hate updating packages on EPEL but I was asked too... It would have been really nice to find out before pushing the packages to stable but no one seems to check updates-testing on EPEL. 

I could try to bump the Epoch and downgrade the package.

Comment 2 Avi Kivity 2018-01-28 14:58:27 UTC
For reference, the offending commit is

commit 03d6e7d67271be698d9a2fd51f749201649e0223
Author: Haydn Trigg <me>
Date:   Sat Jul 25 11:45:10 2015 +0930

    Removed boost requirement from memory.h (detail)
    
    Removed the boost requirement from memory.h using the shared_memory type defined in ptr.h

diff --git a/include/yaml-cpp/node/detail/memory.h b/include/yaml-cpp/node/detail/memory.h
index e3d344b..8f2bc26 100644
--- a/include/yaml-cpp/node/detail/memory.h
+++ b/include/yaml-cpp/node/detail/memory.h
@@ -5,12 +5,10 @@
     (defined(__GNUC__) && (__GNUC__ == 3 && __GNUC_MINOR__ >= 4) || \
      (__GNUC__ >= 4))  // GCC supports "pragma once" correctly since 3.4
 #pragma once
 #endif
 
-#include <boost/shared_ptr.hpp>
-#include <boost/smart_ptr/shared_ptr.hpp>
 #include <set>
 
 #include "yaml-cpp/dll.h"
 #include "yaml-cpp/node/ptr.h"
 
@@ -38,11 +36,11 @@ class YAML_CPP_API memory_holder {
 
   node& create_node() { return m_pMemory->create_node(); }
   void merge(memory_holder& rhs);
 
  private:
-  boost::shared_ptr<memory> m_pMemory;
+  shared_memory m_pMemory;
 };
 }
 }
 
 #endif  // VALUE_DETAIL_MEMORY_H_62B23520_7C8E_11DE_8A39_0800200C9A66


which changes a boost::shared_ptr to an std::shared_ptr, which have different memory layouts.

Comment 3 Avi Kivity 2018-01-28 14:59:39 UTC
Bumping epoch and downgrading would work too. We have some packages that were already built againt 0.5.3, but they weren't released yet.

Comment 4 Richard Shaw 2018-01-28 15:24:31 UTC
For now, what about reversing the patch and doing new builds?

Comment 5 Avi Kivity 2018-01-28 15:29:54 UTC
+1

Comment 6 Richard Shaw 2018-01-28 15:46:21 UTC
Here's a scratch build I just completed, please make sure it address the problem before I do official builds:

https://koji.fedoraproject.org/koji/taskinfo?taskID=24516842

Comment 7 Avi Kivity 2018-01-28 15:49:24 UTC
Will test.

Comment 8 Avi Kivity 2018-01-28 16:05:36 UTC
Scratch build still fails.

Comment 9 Richard Shaw 2018-01-28 17:44:57 UTC
I tried running abi-compliance-checker against them but it errors out, we'll see if I can figure it out.

Comment 10 Richard Shaw 2018-01-28 17:56:28 UTC
See if you can make sense of this:

https://hobbes1069.fedorapeople.org/compat_reports/yaml-cpp/0.5.1_to_0.5.3/compat_report.html

Comment 11 Avi Kivity 2018-01-28 18:43:27 UTC
It does say that node_data changed layout. Since there are inline member functions for that type, such a function inlined in an executable compiled against one version of yaml-cpp will fail against another version.

Comment 12 Richard Shaw 2018-01-29 01:27:02 UTC
Ok, working on reverting the update.

Comment 13 Avi Kivity 2018-01-29 09:16:38 UTC
Thanks. Please also consider packaging the static library in -devel, it will help avoid such problems in the future as upstream doesn't maintain binary compatibility across patch releases (in their defense, that's very hard with template-intensive C++ code).

Comment 14 Fedora Update System 2018-01-29 13:16:00 UTC
yaml-cpp-0.5.1-1.el7.1 pdns-3.4.11-3.el7 mongodb-2.6.12-6.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-adb6af896b

Comment 15 Richard Shaw 2018-01-29 13:51:14 UTC
(In reply to Avi Kivity from comment #13)
> Thanks. Please also consider packaging the static library in -devel, it will
> help avoid such problems in the future as upstream doesn't maintain binary
> compatibility across patch releases (in their defense, that's very hard with
> template-intensive C++ code).

Unfortunately the packaging guidelines require it be in a -static subpackage...

Comment 16 Avi Kivity 2018-01-29 13:51:37 UTC
Thanks, verified that it fixes the problem for me, and that it installs over 0.5.3 without complaints.

Comment 17 Avi Kivity 2018-01-29 13:52:07 UTC
> Unfortunately the packaging guidelines require it be in a -static subpackage...

Not a problem.

Comment 18 Richard Shaw 2018-01-29 13:54:29 UTC
I can push it to stable early if you log in to bodhi and give it positive karma (plus scrounge up 2 more people to do it, with testing!).

Comment 19 Avi Kivity 2018-01-29 13:56:35 UTC
I already Karma'd it. I'll ask here and on our mailing list.

Comment 20 Fedora Update System 2018-01-29 18:44:25 UTC
mongodb-2.6.12-6.el7, pdns-3.4.11-3.el7, yaml-cpp-0.5.1-1.el7.1 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-adb6af896b

Comment 21 Avi Kivity 2018-01-30 09:52:50 UTC
Karma is now at +3 (from at least 4 testers :)

Comment 22 Fedora Update System 2018-01-30 13:57:59 UTC
mongodb-2.6.12-6.el7 pdns-3.4.11-3.el7 yaml-cpp-0.5.1-1.el7.2 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-adb6af896b

Comment 23 Fedora Update System 2018-01-30 20:37:27 UTC
mongodb-2.6.12-6.el7, pdns-3.4.11-3.el7, yaml-cpp-0.5.1-1.el7.2 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-adb6af896b

Comment 24 Avi Kivity 2018-01-31 17:55:14 UTC
So, from all the logs I can't tell if it got pushed to stable or not? It has the necessary karma AFAICT.

Comment 25 Richard Shaw 2018-01-31 18:18:34 UTC
I've submitted it to stable, but it won't get picked up until the next push.

Comment 26 Fedora Update System 2018-01-31 19:52:24 UTC
mongodb-2.6.12-6.el7, pdns-3.4.11-3.el7, yaml-cpp-0.5.1-1.el7.2 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

Comment 27 Avi Kivity 2018-02-01 08:57:52 UTC
New packages showed up. Thanks a lot for your quick response.

Comment 28 Avi Kivity 2018-02-01 08:58:12 UTC
New packages showed up. Thanks a lot for your quick response.