From 31c9a3a1d330606493e5e70aec3cd1a36d8c61a0 Mon Sep 17 00:00:00 2001
From: Nobuyoshi Nakada <nobu@ruby-lang.org>
Date: Tue, 28 May 2024 23:19:33 +0900
Subject: [PATCH] [Bug #20438] Disallow "%\n" and "%\0"

---
 spec/ruby/core/string/modulo_spec.rb | 69 ++++++++++++++++++----------
 sprintf.c                            |  4 --
 test/ruby/test_sprintf.rb            | 11 ++---
 3 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/spec/ruby/core/string/modulo_spec.rb b/spec/ruby/core/string/modulo_spec.rb
index bf96a82874..9045afa263 100644
--- a/spec/ruby/core/string/modulo_spec.rb
+++ b/spec/ruby/core/string/modulo_spec.rb
@@ -55,33 +55,48 @@ describe "String#%" do
     -> { ("foo%" % [])}.should raise_error(ArgumentError)
   end
 
-  it "formats single % character before a newline as literal %" do
-    ("%\n" % []).should == "%\n"
-    ("foo%\n" % []).should == "foo%\n"
-    ("%\n.3f" % 1.2).should == "%\n.3f"
+  ruby_version_is ""..."3.4" do
+    it "formats single % character before a newline as literal %" do
+      ("%\n" % []).should == "%\n"
+      ("foo%\n" % []).should == "foo%\n"
+      ("%\n.3f" % 1.2).should == "%\n.3f"
+    end
+
+    it "formats single % character before a NUL as literal %" do
+      ("%\0" % []).should == "%\0"
+      ("foo%\0" % []).should == "foo%\0"
+      ("%\0.3f" % 1.2).should == "%\0.3f"
+    end
+
+    it "raises an error if single % appears anywhere else" do
+      -> { (" % " % []) }.should raise_error(ArgumentError)
+      -> { ("foo%quux" % []) }.should raise_error(ArgumentError)
+    end
+
+    it "raises an error if NULL or \\n appear anywhere else in the format string" do
+      begin
+        old_debug, $DEBUG = $DEBUG, false
+
+        -> { "%.\n3f" % 1.2 }.should raise_error(ArgumentError)
+        -> { "%.3\nf" % 1.2 }.should raise_error(ArgumentError)
+        -> { "%.\03f" % 1.2 }.should raise_error(ArgumentError)
+        -> { "%.3\0f" % 1.2 }.should raise_error(ArgumentError)
+      ensure
+        $DEBUG = old_debug
+      end
+    end
   end
 
-  it "formats single % character before a NUL as literal %" do
-    ("%\0" % []).should == "%\0"
-    ("foo%\0" % []).should == "foo%\0"
-    ("%\0.3f" % 1.2).should == "%\0.3f"
-  end
-
-  it "raises an error if single % appears anywhere else" do
-    -> { (" % " % []) }.should raise_error(ArgumentError)
-    -> { ("foo%quux" % []) }.should raise_error(ArgumentError)
-  end
-
-  it "raises an error if NULL or \\n appear anywhere else in the format string" do
-    begin
-      old_debug, $DEBUG = $DEBUG, false
-
+  ruby_version_is "3.4" do
+    it "raises an ArgumentError if % is not followed by a conversion specifier" do
+      -> { "%" % [] }.should raise_error(ArgumentError)
+      -> { "%\n" % [] }.should raise_error(ArgumentError)
+      -> { "%\0" % [] }.should raise_error(ArgumentError)
+      -> { " % " % [] }.should raise_error(ArgumentError)
       -> { "%.\n3f" % 1.2 }.should raise_error(ArgumentError)
       -> { "%.3\nf" % 1.2 }.should raise_error(ArgumentError)
       -> { "%.\03f" % 1.2 }.should raise_error(ArgumentError)
       -> { "%.3\0f" % 1.2 }.should raise_error(ArgumentError)
-    ensure
-      $DEBUG = old_debug
     end
   end
 
@@ -125,8 +140,16 @@ describe "String#%" do
     end
   end
 
-  it "replaces trailing absolute argument specifier without type with percent sign" do
-    ("hello %1$" % "foo").should == "hello %"
+  ruby_version_is ""..."3.4" do
+    it "replaces trailing absolute argument specifier without type with percent sign" do
+      ("hello %1$" % "foo").should == "hello %"
+    end
+  end
+
+  ruby_version_is "3.4" do
+    it "raises an ArgumentError if absolute argument specifier is followed by a conversion specifier" do
+      -> { "hello %1$" % "foo" }.should raise_error(ArgumentError)
+    end
   end
 
   it "raises an ArgumentError when given invalid argument specifiers" do
diff --git a/sprintf.c b/sprintf.c
index b2d89617aa..98877d25d2 100644
--- a/sprintf.c
+++ b/sprintf.c
@@ -429,10 +429,6 @@ rb_str_format(int argc, const VALUE *argv, VALUE fmt)
             GETNUM(prec, precision);
             goto retry;
 
-          case '\n':
-          case '\0':
-            p--;
-            /* fall through */
           case '%':
             if (flags != FNONE) {
                 rb_raise(rb_eArgError, "invalid format character - %%");
diff --git a/test/ruby/test_sprintf.rb b/test/ruby/test_sprintf.rb
index c453ecd350..8cf2c63a20 100644
--- a/test/ruby/test_sprintf.rb
+++ b/test/ruby/test_sprintf.rb
@@ -266,8 +266,8 @@ class TestSprintf < Test::Unit::TestCase
     # Specifying the precision multiple times with negative star arguments:
     assert_raise(ArgumentError, "[ruby-core:11570]") {sprintf("%.*.*.*.*f", -1, -1, -1, 5, 1)}
 
-    # Null bytes after percent signs are removed:
-    assert_equal("%\0x hello", sprintf("%\0x hello"), "[ruby-core:11571]")
+    assert_raise(ArgumentError) {sprintf("%\0x hello")}
+    assert_raise(ArgumentError) {sprintf("%\nx hello")}
 
     assert_raise(ArgumentError, "[ruby-core:11573]") {sprintf("%.25555555555555555555555555555555555555s", "hello")}
 
@@ -279,10 +279,9 @@ class TestSprintf < Test::Unit::TestCase
     assert_raise_with_message(ArgumentError, /unnumbered\(1\) mixed with numbered/) { sprintf("%1$*d", 3) }
     assert_raise_with_message(ArgumentError, /unnumbered\(1\) mixed with numbered/) { sprintf("%1$.*d", 3) }
 
-    verbose, $VERBOSE = $VERBOSE, nil
-    assert_nothing_raised { sprintf("", 1) }
-  ensure
-    $VERBOSE = verbose
+    assert_warning(/too many arguments/) do
+      sprintf("", 1)
+    end
   end
 
   def test_float