[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Zachary Turner via lldb-commits]

Can we try using lldb_private::RegularExpression for everything?  (Long
term, adding a new base class method seems like a better approach, but at
least this quick fix is an immediate fix and should be strictly better than
crashing)

On Wed, Dec 13, 2017 at 10:27 AM Aaron Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> asmith added inline comments.
>
>
> ================
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:392-399
> +  bool is_regex = false;
> +  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) {
> +    // Trying to compile an invalid regex could throw an exception.
> +    // Only search by regex when it's valid.
> +    lldb_private::RegularExpression name_regex(name_str);
> +    is_regex = name_regex.IsValid();
> +  }
> ----------------
> asmith wrote:
> > zturner wrote:
> > > I can see two possible problems here.
> > >
> > > 1. Now if it is a valid regex, it's going to compile it twice, hurting
> performance (not sure by how much, it could be negligible)
> > >
> > > 2. Whether or not it's valid is determined by asking LLDB's regex
> engine, but it's then compiled with C++ STL's engine.  It's possible they
> won't agree on whether or not a regex is valid.
> > >
> > > You mentioned that it throws an exception on an invalid regex.  What
> actually happens though?  Because we compile with exception support
> disabled.  Does it terminate the program?
> > >
> > > At a minimum, the validity check and the find function should agree on
> the regex engine.  If the only way to make that work is to change from
> `std::regex` to `lldb_private::RegularExpression` as the matching engine,
> then I guess we have to do that.
> > How about this?
> >
> > ```
> >   lldb_private::RegularExpression name_regex(name_str);
> >   if (name_regex.IsValid())
> >     FindTypesByRegex(name_regex, max_matches, types); // pass regex here
> >   else
> >     FindTypesByName(name_str, max_matches, types);
> >   return types.GetSize();
> > }
> >
> > void SymbolFilePDB::FindTypesByRegex(lldb_private::RegularExpression
> &regex,
> >                                      uint32_t max_matches,
> >                                      lldb_private::TypeMap &types) {
> > ```
> Our experience on Windows is that when lldb is built with exceptions
> disabled, std::regex segfaults on an invalid regex causing lldb to
> terminate.
>
> The test case will reproduce the failure or an example from the lldb
> console (if you had the corresponding changes required to do the symbol
> lookup):
>
> image lookup -n function_name
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D41086
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20171213/e33c2370/attachment.html>