homepage

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?

As it's barely worth fixing, it's not worth backporting.

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?

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.

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'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.

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.

See bpo-31338 for adopting the abort() idiom.

Alright, I'm going to close this bug in favor of bpo-31338