From 672732db2f0c2fe69379eb019b0676554536fac2 Mon Sep 17 00:00:00 2001
From: Bastien Chevreux <bastien.chevreux@dsm.com>
Date: Thu, 8 Sep 2016 09:56:37 -0400
Subject: [PATCH] testprog.cpp: test case and fix for segfaults when using
 OptionGroup

Class OptionParser stored only pointers to option groups when using
add_option_group(). This can lead to the case where arguments are parsed
long after an OptionGroup went out of scope, leading to memory access to
freed memory and often to segmentation faults (the changed testprog.cpp
demonstrates this).

This can easily happen if a parser is configured in an external function
like this
auto parser = optparse::OptionParser();
setupParser(parser);

Fix:
Class OptionParser now stores option groups instead of pointers to them.

Pro:
- least amount of changes to the code
Con:
- Option groups cannot be changed after they were added to the parser
via add_option_group(). Should this be needed, then a bigger rewrite
would be necessary where the parser uses a factory function which
creates and stores new option groups and returns a pointer/reference to
them.
---
 OptionParser.cpp | 10 +++++-----
 OptionParser.h   |  2 +-
 testprog.cpp     | 24 +++++++++++++-----------
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/OptionParser.cpp b/OptionParser.cpp
index 799ffdc..1a3119d 100644
--- a/OptionParser.cpp
+++ b/OptionParser.cpp
@@ -195,7 +195,7 @@ OptionParser& OptionParser::add_option_group(const OptionGroup& group) {
     for (set<string>::const_iterator it = option._long_opts.begin(); it != option._long_opts.end(); ++it)
       _optmap_l[*it] = &option;
   }
-  _groups.push_back(&group);
+  _groups.push_back(group);
   return *this;
 }
 
@@ -322,8 +322,8 @@ Values& OptionParser::parse_args(const vector<string>& v) {
       _values[it->dest()] = it->get_default();
   }
 
-  for (list<OptionGroup const*>::iterator group_it = _groups.begin(); group_it != _groups.end(); ++group_it) {
-    for (list<Option>::const_iterator it = (*group_it)->_opts.begin(); it != (*group_it)->_opts.end(); ++it) {
+  for (list<OptionGroup>::iterator group_it = _groups.begin(); group_it != _groups.end(); ++group_it) {
+    for (list<Option>::const_iterator it = group_it->_opts.begin(); it != group_it->_opts.end(); ++it) {
       if (it->get_default() != "" and not _values.is_set(it->dest()))
         _values[it->dest()] = it->get_default();
     }
@@ -397,8 +397,8 @@ string OptionParser::format_help() const {
   ss << _("Options") << ":" << endl;
   ss << format_option_help();
 
-  for (list<OptionGroup const*>::const_iterator it = _groups.begin(); it != _groups.end(); ++it) {
-    const OptionGroup& group = **it;
+  for (list<OptionGroup>::const_iterator it = _groups.begin(); it != _groups.end(); ++it) {
+    const OptionGroup& group = *it;
     ss << endl << "  " << group.title() << ":" << endl;
     if (group.description() != "") {
       unsigned int malus = 4; // Python seems to not use full length
diff --git a/OptionParser.h b/OptionParser.h
index ccafbc5..20ad53d 100644
--- a/OptionParser.h
+++ b/OptionParser.h
@@ -179,7 +179,7 @@ class OptionParser : public OptionContainer {
     Values _values;
 
     strMap _defaults;
-    std::list<OptionGroup const*> _groups;
+    std::list<OptionGroup> _groups;
 
     std::list<std::string> _remaining;
     std::list<std::string> _leftover;
diff --git a/testprog.cpp b/testprog.cpp
index ac68f1f..db7de86 100644
--- a/testprog.cpp
+++ b/testprog.cpp
@@ -120,17 +120,19 @@ int main(int argc, char *argv[])
   parser.add_option("-K", "--callback") .action("callback") .callback(mc) .help("callback test");
   parser.add_option("--string-callback") .action("callback") .callback(mc) .type("string") .help("callback test");
 
-  OptionGroup group1 = OptionGroup(parser, "Dangerous Options",
-      "Caution: use these options at your own risk. "
-      "It is believed that some of them\nbite.");
-  group1.add_option("-g") .action("store_true") .help("Group option.") .set_default("0");
-  parser.add_option_group(group1);
-
-  OptionGroup group2 = OptionGroup(parser, "Size Options", "Image Size Options.");
-  group2.add_option("-w", "--width") .action("store") .type("int") .set_default(640) .help("default: %default");
-  group2.add_option("--height") .action("store") .type("int") .help("default: %default");
-  parser.set_defaults("height", 480);
-  parser.add_option_group(group2);
+  {
+    OptionGroup group1 = OptionGroup(parser, "Dangerous Options",
+        "Caution: use these options at your own risk. "
+        "It is believed that some of them\nbite.");
+    group1.add_option("-g") .action("store_true") .help("Group option.") .set_default("0");
+    parser.add_option_group(group1);
+
+    OptionGroup group2 = OptionGroup(parser, "Size Options", "Image Size Options.");
+    group2.add_option("-w", "--width") .action("store") .type("int") .set_default(640) .help("default: %default");
+    group2.add_option("--height") .action("store") .type("int") .help("default: %default");
+    parser.set_defaults("height", 480);
+    parser.add_option_group(group2);
+  }
 
   Values& options = parser.parse_args(argc, argv);
   vector<string> args = parser.args();