gh-106263: Fix segfault in `signaldict_repr` #106270

CharlieZhao95 · 2023-06-30T09:05:12Z

Fix potential crash risks in signaldict object.

Issue: Segmentation fault when instantiating decimal.SignalDictMixin type #106263

Lib/test/test_decimal.py

Co-authored-by: sunmy2019 <59365878+sunmy2019@users.noreply.github.com>

sunmy2019 · 2023-06-30T10:16:09Z

Modules/_decimal/_decimal.c

 static int
 signaldict_init(PyObject *self, PyObject *args UNUSED, PyObject *kwds UNUSED)
 {
-    SdFlagAddr(self) = NULL;
+    SdFlagAddr(self) = &dflt_ctx.status;


They will all share the same flags.

Will users start to misuse this construction method, then found in surprise that its flags being changed without notice?

I would prefer forbidding this than silently allowing this.

Thanks! It seems that it is not easy to forbid instantiation directly, because decimal.Context() depends on the signaldict instance. If you have a good idea, please let me know :) Or let's wait for advice from other professionals~

CharlieZhao95 · 2023-06-30T16:44:57Z

At first I considered two solutions to fix this:

Add NULL pointer checker in all places where signaldict is used. If signaldict is NULL , an exception will be thrown (maybe SdFlags macro can help us do this)
Assign default value to signaldict object in the signaldict_init (simplest)

Assuming that there is a class with parameterless constructor, we use this constructor to create an instance of the class, then call the method of the class, and then the program crashes. It sounds weird, even though we don't want users to call this constructor directly.

IMO, it makes sense to properly initialize instance attributes in a parameterless constructor. So I prefer solution 2 for now, it's simple and works. Please correct me if I am wrong :)

sunmy2019 · 2023-07-01T02:29:10Z

IMO, it makes sense to properly initialize instance attributes in a parameterless constructor. So I prefer solution 2 for now, it's simple and works. Please correct me if I am wrong :)

How about this? Disallowing signaldict_setitem if SdFlagAddr(v) == dflt_ctx->flags || .... (edited)

CharlieZhao95 · 2023-07-01T09:33:09Z

How about this? Disallowing signaldict_setitem if SdFlagAddr(v) == &dflt_ctx.

I got you. But this seems to limit users from changing the value of decimal.Context().flags or decimal.Context().traps.

cpython/Modules/_decimal/_decimal.c

Lines 1275 to 1283 in 04dfc6f

    
           if (default_context_template) { 
        
               *ctx = *CTX(default_context_template); 
        
           } 
        
           else { 
        
               *ctx = dflt_ctx; 
        
           } 
        
           SdFlagAddr(self->traps) = &ctx->traps; 
        
           SdFlagAddr(self->flags) = &ctx->status;

That's why we hate global variable.

What if we choose option 1? @sunmy2019 We need to add pointer checker for all methods of signaldict object, not just methods that may crash. Otherwise, the signaldict_len function will always return SIGNAL_MAP_LEN(9), but the actual object is NULL. This will confuse others.

sunmy2019 · 2023-07-01T09:45:23Z

I got you. But this seems to limit users from changing the value of decimal.Context().flags or decimal.Context().traps.

I got you now. I didn't look into it closely.

What if we choose option 1? @sunmy2019 We need to add pointer checker for all methods of signaldict object, not just methods that may crash. Otherwise, the signaldict_len function will always return SIGNAL_MAP_LEN(9), but the actual object is NULL. This will confuse others.

This is what I previously preferred.

Modules/_decimal/_decimal.c

CharlieZhao95 · 2023-07-28T09:34:08Z

@erlend-aasland I updated this PR, would you mind to review this :)

erlend-aasland · 2023-07-30T09:16:38Z

Thanks, Kumar and Charlie! Should this be backported?

sunmy2019 · 2023-07-30T09:41:26Z

I think it should.

CharlieZhao95 · 2023-07-31T07:43:04Z

Thanks, Kumar and Charlie! Should this be backported?

Yes, it should. This backport does not bring any side effects. If there are conflicts, I can manually backport them. :)

miss-islington · 2023-07-31T07:46:05Z

Thanks @CharlieZhao95 for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2023-07-31T07:46:05Z

Thanks @CharlieZhao95 for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington · 2023-07-31T07:46:08Z

Sorry, @CharlieZhao95 and @kumaraditya303, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3979150a0d406707f6d253d7c15fb32c1e005a77 3.12

miss-islington · 2023-07-31T07:46:12Z

Sorry, @CharlieZhao95 and @kumaraditya303, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3979150a0d406707f6d253d7c15fb32c1e005a77 3.11

…le (python#106270) Co-authored-by: sunmy2019 <59365878+sunmy2019@users.noreply.github.com> (cherry picked from commit 3979150)

bedevere-bot · 2023-07-31T08:35:53Z

GH-107490 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2023-07-31T08:38:05Z

GH-107491 is a backport of this pull request to the 3.12 branch.

CharlieZhao95 added 2 commits June 30, 2023 16:27

Fix segfault in signaldict

fb94e45

Add NEWS

b035a5e

CharlieZhao95 requested a review from rhettinger as a code owner June 30, 2023 09:05

bedevere-bot added the awaiting review label Jun 30, 2023

bedevere-bot mentioned this pull request Jun 30, 2023

Segmentation fault when instantiating decimal.SignalDictMixin type #106263

Closed

sunmy2019 reviewed Jun 30, 2023

View reviewed changes

Lib/test/test_decimal.py Outdated Show resolved Hide resolved

Update Lib/test/test_decimal.py

7ec5f15

Co-authored-by: sunmy2019 <59365878+sunmy2019@users.noreply.github.com>

sunmy2019 reviewed Jun 30, 2023

View reviewed changes

rhettinger removed their request for review June 30, 2023 22:36

This comment was marked as outdated.

Sign in to view

Add pointer checker and test cases

773167e

sunmy2019 reviewed Jul 2, 2023

View reviewed changes

Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved

erlend-aasland self-requested a review July 2, 2023 20:35

use value_error_int and value_error_ptr

1880fc1

sunmy2019 reviewed Jul 6, 2023

View reviewed changes

Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved

Replace macro with const statement

d3e24fd

kumaraditya303 approved these changes Jul 21, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 21, 2023

CharlieZhao95 added 2 commits July 28, 2023 15:03

Merge remote-tracking branch 'upstream/main' into decimal/segfault

9fe1360

Fix global variables check

68e03f2

kumaraditya303 approved these changes Jul 30, 2023

View reviewed changes

kumaraditya303 added the extension-modules C modules in the Modules dir label Jul 30, 2023

kumaraditya303 merged commit 3979150 into python:main Jul 30, 2023

bedevere-bot removed the awaiting merge label Jul 30, 2023

erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jul 31, 2023

miss-islington assigned kumaraditya303 Jul 31, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 31, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 31, 2023

Uh oh!

gh-106263: Fix segfault in signaldict_repr #106270

gh-106263: Fix segfault in signaldict_repr #106270

Uh oh!

Conversation

CharlieZhao95 commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sunmy2019 Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunmy2019 Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunmy2019 commented Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharlieZhao95 commented Jul 1, 2023

Uh oh!

This comment was marked as outdated.

sunmy2019 commented Jul 1, 2023

Uh oh!

Uh oh!

Uh oh!

CharlieZhao95 commented Jul 28, 2023

Uh oh!

erlend-aasland commented Jul 30, 2023

Uh oh!

sunmy2019 commented Jul 30, 2023

Uh oh!

CharlieZhao95 commented Jul 31, 2023

Uh oh!

miss-islington commented Jul 31, 2023

Uh oh!

miss-islington commented Jul 31, 2023

Uh oh!

miss-islington commented Jul 31, 2023

Uh oh!

miss-islington commented Jul 31, 2023

Uh oh!

bedevere-bot commented Jul 31, 2023

Uh oh!

bedevere-bot commented Jul 31, 2023

Uh oh!

Uh oh!

gh-106263: Fix segfault in `signaldict_repr` #106270

gh-106263: Fix segfault in `signaldict_repr` #106270

CharlieZhao95 commented Jun 30, 2023 •

edited

Loading

sunmy2019 Jun 30, 2023 •

edited

Loading

CharlieZhao95 Jun 30, 2023 •

edited

Loading

CharlieZhao95 commented Jun 30, 2023 •

edited

Loading

sunmy2019 commented Jul 1, 2023 •

edited

Loading