From 2402c2bad753dcdb9726a1cdf3d676d8829917a1 Mon Sep 17 00:00:00 2001
From: Oblarg <emichaelbarnett@gmail.com>
Date: Fri, 18 Oct 2019 10:57:43 -0400
Subject: [PATCH] Fix C++ command group recursive constructor bug (#1941)

Passing command groups as lvalue-references to other command groups should be illegal, as their copy constructors have been deleted. However, copy constructors are const-qualified. This led to a very obscure bug where passing a command group by lvalue to another command group would result in a valid template expansion 'looking like' a copy constructor, and being preferred to the deleted copy constructor. This would result in constructor recursion (the expanded constructor would, in an attempt to call the copy constructor, call itself), and an eventual segfault when the stack inevitably overflowed.

This fixes the problem by explicitly deleting the problematic constructor signature - attempting to do this now (correctly) generates a compilation error.
---
 .../include/frc2/command/ParallelCommandGroup.h  | 16 +++++++++++++++-
 .../include/frc2/command/ParallelDeadlineGroup.h | 12 ++++++++++++
 .../include/frc2/command/ParallelRaceGroup.h     | 12 ++++++++++++
 .../include/frc2/command/PerpetualCommand.h      | 12 ++++++++++++
 .../native/include/frc2/command/SelectCommand.h  | 12 ++++++++++++
 .../frc2/command/SequentialCommandGroup.h        | 12 ++++++++++++
 6 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/wpilibc/src/main/native/include/frc2/command/ParallelCommandGroup.h b/wpilibc/src/main/native/include/frc2/command/ParallelCommandGroup.h
index 53aca10be0..322e7b1a83 100644
--- a/wpilibc/src/main/native/include/frc2/command/ParallelCommandGroup.h
+++ b/wpilibc/src/main/native/include/frc2/command/ParallelCommandGroup.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <memory>
 #include <unordered_map>
 #include <utility>
@@ -57,7 +62,12 @@ class ParallelCommandGroup
   // No copy constructors for commandgroups
   ParallelCommandGroup(const ParallelCommandGroup&) = delete;
 
-  template <class... Types>
+  // Prevent template expansion from emulating copy ctor
+  ParallelCommandGroup(ParallelCommandGroup&) = delete;
+
+  template <class... Types,
+            typename = std::enable_if_t<std::conjunction_v<
+                std::is_base_of<Command, std::remove_reference_t<Types>>...>>>
   void AddCommands(Types&&... commands) {
     std::vector<std::unique_ptr<Command>> foo;
     ((void)foo.emplace_back(std::make_unique<std::remove_reference_t<Types>>(
@@ -84,3 +94,7 @@ class ParallelCommandGroup
   bool isRunning = false;
 };
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif
diff --git a/wpilibc/src/main/native/include/frc2/command/ParallelDeadlineGroup.h b/wpilibc/src/main/native/include/frc2/command/ParallelDeadlineGroup.h
index d453ab5f2e..168d0f8525 100644
--- a/wpilibc/src/main/native/include/frc2/command/ParallelDeadlineGroup.h
+++ b/wpilibc/src/main/native/include/frc2/command/ParallelDeadlineGroup.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <memory>
 #include <unordered_map>
 #include <utility>
@@ -65,6 +70,9 @@ class ParallelDeadlineGroup
   // No copy constructors for command groups
   ParallelDeadlineGroup(const ParallelDeadlineGroup&) = delete;
 
+  // Prevent template expansion from emulating copy ctor
+  ParallelDeadlineGroup(ParallelDeadlineGroup&) = delete;
+
   template <class... Types,
             typename = std::enable_if_t<std::conjunction_v<
                 std::is_base_of<Command, std::remove_reference_t<Types>>...>>>
@@ -97,3 +105,7 @@ class ParallelDeadlineGroup
   bool isRunning = false;
 };
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif
diff --git a/wpilibc/src/main/native/include/frc2/command/ParallelRaceGroup.h b/wpilibc/src/main/native/include/frc2/command/ParallelRaceGroup.h
index ba249cf5a5..d7411e9cbe 100644
--- a/wpilibc/src/main/native/include/frc2/command/ParallelRaceGroup.h
+++ b/wpilibc/src/main/native/include/frc2/command/ParallelRaceGroup.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <memory>
 #include <set>
 #include <unordered_map>
@@ -48,6 +53,9 @@ class ParallelRaceGroup
   // No copy constructors for command groups
   ParallelRaceGroup(const ParallelRaceGroup&) = delete;
 
+  // Prevent template expansion from emulating copy ctor
+  ParallelRaceGroup(ParallelRaceGroup&) = delete;
+
   template <class... Types>
   void AddCommands(Types&&... commands) {
     std::vector<std::unique_ptr<Command>> foo;
@@ -76,3 +84,7 @@ class ParallelRaceGroup
   bool isRunning = false;
 };
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif
diff --git a/wpilibc/src/main/native/include/frc2/command/PerpetualCommand.h b/wpilibc/src/main/native/include/frc2/command/PerpetualCommand.h
index c5e8407207..3f3c9e7b79 100644
--- a/wpilibc/src/main/native/include/frc2/command/PerpetualCommand.h
+++ b/wpilibc/src/main/native/include/frc2/command/PerpetualCommand.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <memory>
 #include <utility>
 
@@ -54,6 +59,9 @@ class PerpetualCommand : public CommandHelper<CommandBase, PerpetualCommand> {
   // No copy constructors for command groups
   PerpetualCommand(const PerpetualCommand& other) = delete;
 
+  // Prevent template expansion from emulating copy ctor
+  PerpetualCommand(PerpetualCommand&) = delete;
+
   void Initialize() override;
 
   void Execute() override;
@@ -64,3 +72,7 @@ class PerpetualCommand : public CommandHelper<CommandBase, PerpetualCommand> {
   std::unique_ptr<Command> m_command;
 };
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif
diff --git a/wpilibc/src/main/native/include/frc2/command/SelectCommand.h b/wpilibc/src/main/native/include/frc2/command/SelectCommand.h
index 903b8d159a..8dba378d12 100644
--- a/wpilibc/src/main/native/include/frc2/command/SelectCommand.h
+++ b/wpilibc/src/main/native/include/frc2/command/SelectCommand.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <memory>
 #include <unordered_map>
 #include <utility>
@@ -88,6 +93,9 @@ class SelectCommand : public CommandHelper<CommandBase, SelectCommand<Key>> {
   // No copy constructors for command groups
   SelectCommand(const SelectCommand& other) = delete;
 
+  // Prevent template expansion from emulating copy ctor
+  SelectCommand(SelectCommand&) = delete;
+
   /**
    * Creates a new selectcommand.
    *
@@ -139,3 +147,7 @@ void SelectCommand<T>::Initialize() {
 }
 
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif
diff --git a/wpilibc/src/main/native/include/frc2/command/SequentialCommandGroup.h b/wpilibc/src/main/native/include/frc2/command/SequentialCommandGroup.h
index ebb1b4dd43..dd1f2ce47f 100644
--- a/wpilibc/src/main/native/include/frc2/command/SequentialCommandGroup.h
+++ b/wpilibc/src/main/native/include/frc2/command/SequentialCommandGroup.h
@@ -7,6 +7,11 @@
 
 #pragma once
 
+#ifdef _WIN32
+#pragma warning(push)
+#pragma warning(disable : 4521)
+#endif
+
 #include <limits>
 #include <memory>
 #include <utility>
@@ -61,6 +66,9 @@ class SequentialCommandGroup
   // No copy constructors for command groups
   SequentialCommandGroup(const SequentialCommandGroup&) = delete;
 
+  // Prevent template expansion from emulating copy ctor
+  SequentialCommandGroup(SequentialCommandGroup&) = delete;
+
   template <class... Types,
             typename = std::enable_if_t<std::conjunction_v<
                 std::is_base_of<Command, std::remove_reference_t<Types>>...>>>
@@ -90,3 +98,7 @@ class SequentialCommandGroup
   bool m_runWhenDisabled{true};
 };
 }  // namespace frc2
+
+#ifdef _WIN32
+#pragma warning(pop)
+#endif