Re: [PATCH] Change some arguments to gdb::string_view instead of name+len

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Tue, Oct 1, 2019 at 2:10 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 10/1/19 7:27 PM, Christian Biesinger via gdb-patches wrote:
> > On Tue, Oct 1, 2019 at 1:23 PM Pedro Alves <palves@redhat.com> wrote:
> >>
> >> On 10/1/19 6:33 PM, Christian Biesinger via gdb-patches wrote:
> >>> -  if (linkage_name[len] != '\0')
> >>> +  /* Don't use string_view::operator[] because we are accessing beyond
> >>> +     the size of the string_view, which is technically unsupported.  */
> >>> +  if (linkage_name.data ()[linkage_name.length ()] != '\0')
> >>>      {
> >>>        char *alloc_name;
> >>
> >> It's more than just unsupported, it's undefined behavior.  If we're promising
> >> the string_view interface, then it's supposedly valid to pass in a string_view
> >> that happens to point just at the end of a page, with the one-past-the-end
> >> byte living in an unmapped page.  Dereferencing the one-past-end byte in
> >> that case SIGSEGVs.
> >
> > That's true (though also a pre-existing issue).
> >
> >>> -  if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
> >>> +  if (ms_type == mst_file_text && startswith (name.data (), "__gnu_compiled"))
> >>>      return (NULL);
> >>>
> >>
> >> This, via startswith also assumes that name.data() is a null-terminated
> >> string.
> >
> > Ah yes. I'll add a startswith version that takes string_views.
> >
> >> I wonder whether we should have a zstring_view type.  like string_view, but
> >> assumes/requires null-terminated.
> >
> > How does that solve anything? This function can (apparently) take
> > non-null terminated strings, so zstring_view wouldn't work?
>
> Ah, right.
>
> Hmm.
>
> I wonder then, I assume that the caller up the stack should know whether
> the string was originally null terminated?  I wonder about tweaking the
> interface to pass that info down somehow.

Actually, let me come at this from a different direction...

I looked at all the callers. In every case, they either passed a
nullterminated string or a substring of an existing string. In other
words, accessing name[length] was always valid, even if the
string_view wasn't nullterminated.

In light of that, how do you feel about just documenting that callers
have to pass a string view where accessing [length] is guaranteed not
to be an invalid memory access?

Thanks,
Christian