Skip to content

BUG,MAINT: Fix utf-8 character stripping memory access - #26919

Merged
ngoldbaum merged 2 commits into
numpy:mainfrom
seberg:fix-strip
Jul 12, 2024
Merged

BUG,MAINT: Fix utf-8 character stripping memory access#26919
ngoldbaum merged 2 commits into
numpy:mainfrom
seberg:fix-strip

Conversation

@seberg

@seberg seberg commented Jul 12, 2024

Copy link
Copy Markdown
Member

This fixes the memory access bug, the old if was there for a reason (obviously...), but unfortunately only the sanitizer checks noticed that.

But to make it clear, I had to just also rename/change things a bit making it unsigned and using a stop range rather than the actual range is just much clearer here where the range can go to length 0 IMO.

I.e. the old code had to check for j >= 0 not j > 0 (j being the last character index), because j could go negative.

Fixes the sanitizer tests.

@seberg
BUG,MAINT: Fix utf-8 character stripping memory access
afe4598 
This fixes the memory access bug, the old if was there for a reason
(obviously...), but unfortunately only the sanitizer checks noticed
that.

But to make it clear, I had to just also rename/change things a bit
making it unsigned and using a stop range rather than the actual
range is just much clearer here where the range can go to length 0
IMO.

I.e. the old code had to check for `j >= 0` not `j > 0` (j being the
last character index), because `j` could go negative.

Fixes the sanitizer tests.
@seberg seberg added 09 - Backport-Candidate PRs tagged should be backported 00 - Bug and removed 00 - Bug labels Jul 12, 2024
@seberg
seberg requested a review from lysnikolaou July 12, 2024 07:12
@mattip

mattip commented Jul 12, 2024

Copy link
Copy Markdown
Member

Is there an easy way to write a test for this?

@seberg

seberg commented Jul 12, 2024

Copy link
Copy Markdown
Member Author

One of the test ran into it with asan, so there is some test. Can't think of a way to make that much more sensitive, presumably valgrind would also catch it.

Since all that is happening is reading 1-3 bytes in front of an allocation and that probably just doesn't fail that reliably.

@ngoldbaum

ngoldbaum commented Jul 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I agree this is clearer and after going through it carefully I think the code behaves identically before and after the renaming (except the re-added if block).

Right now the whitespace variants aren't being tested. The following patch fixes that and adds a test case too.

diff --git a/numpy/_core/tests/test_strings.py b/numpy/_core/tests/test_strings.py
index ec8f8f723e..eb655c129a 100644
--- a/numpy/_core/tests/test_strings.py
+++ b/numpy/_core/tests/test_strings.py
@@ -467,10 +467,12 @@ def test_endswith(self, a, suffix, start, end, out, dt):
     ])
     def test_lstrip(self, a, chars, out, dt):
         a = np.array(a, dtype=dt)
+        out = np.array(out, dtype=dt)
         if chars is not None:
             chars = np.array(chars, dtype=dt)
-        out = np.array(out, dtype=dt)
-        assert_array_equal(np.strings.lstrip(a, chars), out)
+            assert_array_equal(np.strings.lstrip(a, chars), out)
+        else:
+            assert_array_equal(np.strings.lstrip(a), out)

     @pytest.mark.parametrize("a,chars,out", [
         ("", None, ""),
@@ -486,6 +488,7 @@ def test_lstrip(self, a, chars, out, dt):
         ("xyzzyhelloxyzzy", "xyz", "xyzzyhello"),
         ("hello", "xyz", "hello"),
         ("xyxz", "xyxz", ""),
+        ("    ", None, ""),
         ("xyxzx", "x", "xyxz"),
         (["xyzzyhelloxyzzy", "hello"], ["xyz", "xyz"],
          ["xyzzyhello", "hello"]),
@@ -493,10 +496,12 @@ def test_lstrip(self, a, chars, out, dt):
     ])
     def test_rstrip(self, a, chars, out, dt):
         a = np.array(a, dtype=dt)
+        out = np.array(out, dtype=dt)
         if chars is not None:
             chars = np.array(chars, dtype=dt)
-        out = np.array(out, dtype=dt)
-        assert_array_equal(np.strings.rstrip(a, chars), out)
+            assert_array_equal(np.strings.rstrip(a, chars), out)
+        else:
+            assert_array_equal(np.strings.rstrip(a), out)

     @pytest.mark.parametrize("a,chars,out", [
         ("", None, ""),

@ngoldbaum

ngoldbaum commented Jul 12, 2024

Copy link
Copy Markdown
Member

Thanks Sebastian! Let's pull this in to unbreak the ASAN tests. I'll try to fix the issue @WarrenWeckesser reported about stripping unicode in a followup.

@ngoldbaum
ngoldbaum merged commit 727f082 into numpy:main Jul 12, 2024
@seberg
seberg deleted the fix-strip branch July 12, 2024 18:11
@lysnikolaou

lysnikolaou commented Jul 13, 2024

Copy link
Copy Markdown
Member

Thanks both for looking into this and sorry for not being careful enough in my PR.

@charris charris mentioned this pull request Jul 17, 2024
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 17, 2024
@riku-sakamoto riku-sakamoto mentioned this pull request Mar 30, 2026
Closed
@sauravsingla sauravsingla mentioned this pull request Oct 23, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

Assignees

No one assigned

Labels

00 - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@seberg @mattip @ngoldbaum @lysnikolaou @charris