classification
Title: Small opportunity for NULL dereference in compile.c
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: barry Nosy List: barry, serhiy.storchaka, skrah
Priority: normal Keywords:

Created on 2017-09-04 16:43 by barry, last changed 2017-09-06 00:42 by barry. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3282 closed barry, 2017-09-04 16:57
Messages (9)
msg301222 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 16:43
There is a very minor opportunity for NULL dereference in compile.c.  compiler_subdict() does not check the return value of get_const_value(), which could be NULL.  This was found by Kirit Sankar Gupta.

This is not a security issue in practice, since compiler_subdict() calls are_all_items_const() before it gets to the call, so the condition which triggers get_const_value() to return NULL will never happen (i.e. the default: clause of get_const_value()).  Still, it can't hurt to be more correct in case the conditions which are implicitly assumed could change.  Plus the fix is super easy, so why not do it?
msg301223 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 16:47
As it's barely worth fixing, it's not worth backporting.
msg301224 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-04 16:52
The default case is added just for silencing compiler warning. It is never executed. There are a number of places in the core that look like

    assert(0);
    return NULL; /* or whatever */

This is a dead code, but compilers complain without it.

How do you suggest to improve this code?
msg301225 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 17:07
I'll preface that it's not a major issue that I feel *has* to be fixed, but given that assert *can* be compiled away, does it make sense to use abort() instead?  E.g.

1 file changed, 2 insertions(+), 2 deletions(-)
Python/compile.c | 4 ++--

modified   Python/compile.c
@@ -1350,8 +1350,8 @@ get_const_value(expr_ty e)
     case NameConstant_kind:
         return e->v.NameConstant.value;
     default:
-        assert(!is_const(e));
-        return NULL;
+        /* We should never get here. */
+        abort();
     }
 }
 

This at least makes gcc happy and makes the intent clearer.
msg301228 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-04 17:18
Could you please also look at other asserts? I have counted 48 occurrences of assert(0), 11 assert(0 && "message") and 2 assert(!"message"). If fix one occurrence, why not fix all others?
msg301229 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-09-04 17:20
I'm very much in favor of using abort() /* NOT REACHED */ in such cases.

The only drawback is that in the case of libraries, sometimes
distribution package lint tools complain.
msg301234 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 18:06
On Sep 4, 2017, at 10:18, Serhiy Storchaka <report@bugs.python.org> wrote:
> Serhiy Storchaka added the comment:
> 
> Could you please also look at other asserts? I have counted 48 occurrences of assert(0), 11 assert(0 && "message") and 2 assert(!"message"). If fix one occurrence, why not fix all others?

I think it makes sense to change them all, probably using the pattern that Stefan gave, but let’s do that in a separate issue.
msg301245 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 19:19
See bpo-31338 for adopting the abort() idiom.
msg301417 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-06 00:42
Alright, I'm going to close this bug in favor of bpo-31338
History
Date User Action Args
2017-09-06 00:42:50barrysetstatus: open -> closed
resolution: wont fix
messages: + msg301417

stage: resolved
2017-09-04 19:19:00barrysetmessages: + msg301245
2017-09-04 18:06:56barrysetmessages: + msg301234
2017-09-04 17:20:47skrahsetnosy: + skrah
messages: + msg301229
2017-09-04 17:18:37serhiy.storchakasetmessages: + msg301228
2017-09-04 17:07:16barrysetmessages: + msg301225
2017-09-04 16:57:30barrysetpull_requests: + pull_request3325
2017-09-04 16:52:23serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg301224
2017-09-04 16:47:01barrysetmessages: + msg301223
2017-09-04 16:43:32barrycreate