| msg301222 - (view) |
Author: Barry A. Warsaw (barry) *  |
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) * |
Date: 2017-09-04 16:47 |
| As it's barely worth fixing, it's not worth backporting. |
|
|
| msg301224 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-09-04 19:19 |
| See bpo-31338 for adopting the abort() idiom. |
|
|
| msg301417 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2017-09-06 00:42 |
| Alright, I'm going to close this bug in favor of bpo-31338 |
|
|