[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D105169#3069555 <https://reviews.llvm.org/D105169#3069555>, @aqjune wrote:

> I see, the crash is happening at the line because SimplifyCFG removes the `sti->index_entries` null pointer check (which seems valid to me).
> If `stl->index_entries` was null, it would lead to uses of uninitialized variables as function arguments, which is UB.
> SimplifyCFG relies on the information and removes the `stl->index_entries` null check.

Thanks! That explains it. Although the actual crash is due to yet another removed condition:

  int64_t pos_min=pos_min, pos_max=pos_max, pos, pos_limit; // pos_min and pos_max are undef
  ...
  if (sti->index_entries) {
     index = av_index_search_timestamp();
     if (index >= 0) {
         ... (dereference sti->index_entries+index)
         ... (initialize pos_min and pos_max)
     }
  }
  // If sti->index_entries was NULL, UB must happen at the call below because undef is passed to ff_gen_search's noundef arg.
  pos = ff_gen_search(s, stream_index, target_ts, pos_min, pos_max, pos_limit,
                       ts_min, ts_max, flags, &ts, avif->read_timestamp);

Both of the nested if conditions are removed:

  int64_t pos_min=pos_min, pos_max=pos_max, pos, pos_limit;
  ...
  if (true) { // Changed to true!
     index = av_index_search_timestamp();
     if (true) { // Also changed to true
         ... (dereference sti->index_entries+index) // This can crash with index = -1
         ... (initialize pos_min and pos_max)
     }
  }
  pos = ff_gen_search(s, stream_index, target_ts, pos_min, pos_max, pos_limit,
                       ts_min, ts_max, flags, &ts, avif->read_timestamp);



> I think the solution is to initialize `pos_min` and `pos_max` to some value.
> If `sti->index_entries` is null, they are never used inside `ff_gen_search` anyway, so initializing it into any value (e.g. 0) will work.

Yes, any value should be fine (and I guess 0 is the easiest to optimize for the compiler). Just as background - this is in a codebase that really tries to avoid default variable initializations unless they're proven to be necessary. But here they clearly are due to the UB rules of the language.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105169/new/

https://reviews.llvm.org/D105169