Safer alternatives for wcscpy_s

[Claudio DeSouza]

We have various cases of UNSAFE_TODO in chromium relating to wcscpy_s. One example I came across looks like this:

void StatusIconWin::SetToolTip(const std::u16string& tool_tip) {
  // Create the icon.
  NOTIFYICONDATA icon_data;
  InitIconData(&icon_data);
  icon_data.uFlags = NIF_TIP;
  UNSAFE_TODO(wcscpy_s(icon_data.szTip, base::as_wcstr(tool_tip)));
  BOOL result = Shell_NotifyIcon(NIM_MODIFY, &icon_data);
  if (!result) {
    LOG(WARNING) << "Unable to set tooltip for status tray icon";
  }
}

I think we should have some safer replacement for this type of copy, and actually I was quite surprised to find out that span's `copy_from` doesn't have any checks for buffer size. The following code, for instance, is unsafe:

base::as_writable_byte_span(

  icon_data->szTip).copy_from_nonoverlapping(base::as_byte_span(tool_tip));

I'm raising this question here, to gather suggestions/feedback of what sort of extension we could use to handle cases like this. Maybe something like cstring::try_to_copy_to?

Claudio

[Thomas Sepez]

Yes, the bounds are checked, see https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span.h;drc=f0cb0ae3f9b142a11fdc5efc77e27a5d53b6b6cf;l=1048

Some confusion might arise because there are other forms of copy_from() that do omit this check, but these can only be instantiated when the compiler can prove the sizes are identical at compile time.

--
You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/b8f29b12-c326-49bd-bd98-aa1c13e690c3n%40chromium.org.

[Claudio DeSouza]

I don't think that the bound check alone is what we want if we are going to fix uses of wcscpy_s others similar to it. With wcscpy_s the copy bails out if the size is larger than the buffer, which leaves the buffer with its zeroed out value.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAPYU9wTF6XFXo0x5NM4v4yWDgOfZFOQ_3%3DFvqBOf_O_0d%2BKwsA%40mail.gmail.com.

[Greg Thompson]

Since we install an invalid parameter handler to crash the process, we'll simply tip over if we ever call `wcscpy_s` with a src that's too long for the dest. Do we need a safe replacement for `wcscpy_s` that will likewise crash us if the dest is too small?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAC2Tj2sSLOBQbuzvx04TVa%2BjBD%2BKz_mjF_g%2BDpKkPoCCj19L-A%40mail.gmail.com.

[Joe Mason]

https://en.cppreference.com/w/c/string/wide/wcscpy says that wcscpy_s calls the constraint handler, not the invalid parameter handler. (But I don't know the relationship between those, maybe that's the same thing?) Are you sure it crashes if the src is too long for the dest? That may well be clang's default constraint handler.

Regardless, it seems like wcscpy_s handles 7 possibilities:

1. dest is longer than src -> ok, end of dest is "clobbered with unspecified values"
2. dest is shorter than src -> crash or do nothing
3. dest is the same length as src, but that length is 0 -> crash or do nothing
4. dest is the same length as src, but that length is over RSIZE_MAX ->  crash or do nothing
5. dest is the same length as src and in-range, and they don't overlap -> ok
6. dest is the same length as src and in-range, but they overlap -> crash or do nothing
7. src or dest are nullptr -> crash or do nothing

Case 1 doesn't seem great, honestly.

Whereas span::copy_from / copy_from_nonoverlapping / copy_prefix_from handles these as:

1. crash, except copy_prefix_from which leaves the end of dest unchanged
2. crash
3. do nothing
4. ok? I don't think span has any max size
5. ok
6. copy_from / copy_prefix_from: ok but less efficient; copy_from_nonoverlapping: undefined behaviour
7. ok if src and dest length are both 0; otherwise DCHECK's when constructing the span

So the question is, are you ok with overlapping ranges working but less efficiently, or do you need a hard check on this? For the other outcomes, copy_prefix_from seems like a fair replacement to me.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAKQnr0SSeoxf6Z6GvWrckojZB3NL%2B4QA5R534Yhk8eHYL_1wdw%40mail.gmail.com.

[Thomas Sepez]

We may need some new replacements here.  Generally, we expect there will be some friction where we run up against an OS-provided C struct, and our strategy in general has been to pass safe C++ types as far down as possible, wrapping in UNSAFE_BUFFERS() with a // SAFTEY: comment at the lowest points. This may be the option here.

Alternativley, we'd want cstring_view.h to cover these cases where copying NULs is important (specifically u16cstring_view for this case.  This should be implicitly convertible to from u16 strings), so the argument to your function could change to this.

At that point, byte_span_with_nul_from_cstring_view() gets us a src for the copy. But you're right there is no copy-if-fits, so currently one has to check the size manually, or hit a CHECK()  in copy_prefix_from(). But it sounds like you want a safe crash if the dest is too small.

Putting this all together, we get

void StatusIconWin::SetToolTip(u16cstring_view tool_tip) {
  // ...

  auto src = byte_span_with_nul_from_cstring_view(tool_tip);

  auto dst = base::as_writable_byte_span(icon_data.szTip);
  dst.copy_prefix_from(src);

[Claudio DeSouza]

I guess my question is then: If the excerpt by Thomas is the functional equivalent in intent to what we are doing with `wcscpy_s` there, should we maybe have extensions to `cstring_view` as this is meant to guarantee null termination too?

  auto src = byte_span_with_nul_from_cstring_view(tool_tip);

  auto dst = base::as_writable_byte_span(icon_data.szTip);
  dst.copy_prefix_from(src);

This seems a bit of a mouthful, and quite not as readable as something like `view.copy_to(dst)`.

Claudio.