CefStringBase::DetachToUserFree modifies data it does not own

Issue #3309 resolved
zhouzuoji created an issue

  userfree_struct_type DetachToUserFree() {
    if (empty())
      return NULL;

    userfree_struct_type str = traits::userfree_alloc();
    memcpy(str, string_, sizeof(struct_type));

    // Free this class' structure but not the data.
    memset(string_, 0, sizeof(struct_type));
    ClearAndFree();

    return str;
  }

memset(string_, 0, sizeof(struct_type));

This line fill zeroes tostring_, which it may not own.

this is the reason why CefRequestContext::GetCachePath() behaves correctly only at first call, and return empty string at later calls.

Comments (6)

  1. Marshall Greenblatt

    I believe the problem is this:

    CefString CefRequestContextImpl::GetCachePath() {
      return CefString(&config_.settings.cache_path);
    }
    

    Which uses this CefString constructor:

      ///
      // Create a new string referencing an existing string structure without taking
      // ownership. Referenced structures must exist for the lifetime of this class
      // and will not be freed by this class.
      ///
      CefStringBase(const struct_type* src) : string_(NULL), owner_(false) {
        if (!src)
          return;
        // Reference the existing structure without taking ownership.
        Attach(const_cast<struct_type*>(src), false);
      }
    

    So the CefString returned from GetCachePath references config_.settings.cache_path instead of making a copy. Then, the call to DetachToUserFree unintentionally clears config_.settings.cache_path.

  2. Marshall Greenblatt

    I think the correct (general) solution is for DetachToUserFree to copy strings instead of transferring ownership when owner_==false.

  3. Marshall Greenblatt

    Fix unintentional state transfer in DetachToUserFree (fixes issue #3309)

    Calling DetachToUserFree() on a CefString holding a reference should copy the value instead of transferring ownership.

    A new StringTest.Ownership test has been added for this behavior.

    → <<cset 4921dc22135e>>

  4. Marshall Greenblatt

    Fix unintentional state transfer in DetachToUserFree (fixes issue #3309)

    Calling DetachToUserFree() on a CefString holding a reference should copy the value instead of transferring ownership.

    A new StringTest.Ownership test has been added for this behavior.

    → <<cset e56440898e62>>

  5. Marshall Greenblatt

    Fix unintentional state transfer in DetachToUserFree (fixes issue #3309)

    Calling DetachToUserFree() on a CefString holding a reference should copy the value instead of transferring ownership.

    A new StringTest.Ownership test has been added for this behavior.

    → <<cset c36c371f68c0>>

  6. Log in to comment