Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix irrString use-after-free with char-like assignment (operator=) #15213

Merged

Conversation

swagtoy
Copy link
Contributor

@swagtoy swagtoy commented Sep 30, 2024

Thank you @appgurueu for suggesting use of resizing instead of making a temporary copy.

Technically fixes #15211, but I'd still like to greenlight that PR for merging.

This is a use-after-free. It is probably a bit serious but otherwise nothing dangerous as it's re-size dependent. This operator is used a lot throughout irrlicht.

My old code

Here's my old code which does a copy, if you need it.

template <class B>
string<T> &operator=(const B *const c)
{
  if (!c) {
	  clear();
	  return *this;
  }
  
  // no longer allowed!
  _IRR_DEBUG_BREAK_IF((void *)c == (void *)c_str());
  
  u32 len = calclen(c);
  B *copy = new B[len];
  std::copy(c, c+len, copy);
  str.resize(len);
  for (u32 l = 0; l < len; ++l)
      str[l] = (T)copy[l];
        
  delete [] copy;
  
  return *this;
}

@sfan5 sfan5 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Bugfix 🐛 PRs that fix a bug labels Sep 30, 2024
Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be good

@swagtoy

This comment was marked as outdated.

@swagtoy
Copy link
Contributor Author

swagtoy commented Sep 30, 2024

Once someone tests the early return (I did and it works), then it's ready to merge 👍

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the logic I want to see. Bringing examples/AutomatedTest/test_string.cpp back could be considered but wouldn't be very useful given that we want to get rid of core::string anyways.

@swagtoy
Copy link
Contributor Author

swagtoy commented Sep 30, 2024

Im definitely against bringing the tests back for now. Let's move to our own string utility soon :)

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 1, 2024

It's ready for merge when anyone is ready

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 1, 2024
@SmallJoker
Copy link
Member

Why do we need template <class B> ? Proper conversions should be done using wide_to_utf8 and utf8_to_wide. There's still string<T>::string(const T *const c, u32 length) (currently also using typename B) to copy as much data as needed - without strange trimming.

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 1, 2024

I'd say its more of a compatibility thing. It's just leftover junk from Irrlicht, so I don't want to bother wiping it over.

But I personally wouldn't have considered any of this and would've just hacked in utf8 checking

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 1, 2024

I'll fix changes when I get off work

@appgurueu
Copy link
Contributor

appgurueu commented Oct 1, 2024

Why do we need template <class B> ?

It's not trivial to remove (and needn't be refactored in this PR) because it's used for unsigned - signed conversions (which don't do anything though, essentially it's just a type cast). For example unsigned char to signed char. But that case is covered by checking for equal sizeofs so sfan5's code is good.

@sfan5
Copy link
Collaborator

sfan5 commented Oct 1, 2024

because it's used for unsigned - signed conversions (which don't do anything though, essentially it's just a type cast)

I think it's used for some char -> wchar_t "conversions" too. Otherwise it would be easy to restrict.

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 2, 2024

Fixed kings. Sorry I can't git.

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 3, 2024

It's ready, for real, I hope.

@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 4, 2024

It's ready, for real, I hope.

@sfan5 sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 4, 2024
@sfan5 sfan5 merged commit 05cbd84 into luanti-org:master Oct 4, 2024
14 of 15 checks passed
@swagtoy
Copy link
Contributor Author

swagtoy commented Oct 4, 2024

Tysm sfan for force pushing those, i was getting tired of this PR 😄

Thank you all for all the feedback and advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants