Skip to content

fix segfault when input shorter than all patterns#6071

Closed
ebernhardson wants to merge 1 commit intofacebook:masterfrom
ebernhardson:fix-strtr
Closed

fix segfault when input shorter than all patterns#6071
ebernhardson wants to merge 1 commit intofacebook:masterfrom
ebernhardson:fix-strtr

Conversation

@ebernhardson
Copy link
Contributor

When the input string is shorter than the shortest input
pattern the last position to visit is set to -1. Patch
short circuits and returns input string when all patterns
are longer than the input string.

@facebook-github-bot
Copy link
Contributor

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D44973

@atdt
Copy link
Contributor

atdt commented Aug 20, 2015

Reproduction case for the crash that this patch fixes:

<?php
$map = array(
  '' => 'ٱ', '' => 'ٱ', '' => 'ٻ', '' => 'ٻ', '' => 'ٻ', '' => 'ٻ', '' => 'پ',
  '' => 'پ', '' => 'پ', '' => 'پ', '' => 'ڀ', '' => 'ڀ', '' => 'ڀ', '' => 'ڀ',
  '' => 'ٺ', '' => 'ٺ', '' => 'ٺ', '' => 'ٺ', '' => 'ٿ', '' => 'ٿ', '' => 'ٿ',
  '' => 'ٿ', '' => 'ٹ', '' => 'ٹ', '' => 'ٹ', '' => 'ٹ', '' => 'ڤ', '' => 'ڤ',
  '' => 'ڤ', '' => 'ڤ', '' => 'ڦ', '' => 'ڦ', '' => 'ڦ', '' => 'ڦ', '' => 'ڄ',
  '' => 'ڄ', '' => 'ڄ', '' => 'ڄ', '' => 'ڃ', '' => 'ڃ', '' => 'ڃ', '' => 'ڃ',
  '' => 'چ', '' => 'چ', '' => 'چ', '' => 'چ', '' => 'ڇ', 'ﭿ' => 'ڇ', '' => 'ڇ',
  '' => 'ڇ', '' => 'ڍ', '' => 'ڍ', '' => 'ڌ', '' => 'ڌ', '' => 'ڎ', '' => 'ڎ',
  '' => 'ڈ', '' => 'ڈ', '' => 'ژ', '' => 'ژ', '' => 'ڑ', '' => 'ڑ', '' => 'ک',
  '' => 'ک', '' => 'ک',
);
strtr( 'ab', $map );

@ebernhardson ebernhardson force-pushed the fix-strtr branch 3 times, most recently from 1737868 to 1c4d1dc Compare August 21, 2015 00:13
@ebernhardson
Copy link
Contributor Author

apologies for not finishing this up yet, still seeing an occasional crash in this code in prod and have not traced it down yet.

@fredemmott
Copy link
Contributor

We close pull requests after changes are requested on phabricator for > 1 week. Will re-open when changes are made.

https://github.com/facebook/hhvm/wiki/Human-Timeouts

@jwatzman
Copy link
Contributor

Author is coming back to this, per discussion on #6384

@jwatzman jwatzman reopened this Oct 19, 2015
When the input string is shorter than the shortest input
pattern the last position to visit is set to a negative, which
due to the unsigned nature rolls over to a very big number.
Fix short circuits and returns input string when all patterns
are longer than the input string.

The LRU cache is holding onto StringData between requests through
the PatAndRepl class, but StringData uses the request-local
allocator. To ensure the patterns and replacements survive
across requests they are now copied into a std::string.

Additionally this included a potential race condition under
concurrency. initTables() could be running in one thread when
the WuManberReplacement is pulled from the LRU cache.  This
second execution could start running the replacement before
initTables() has completed. The class has been adjusted so
it can be declared const at construction and does not change
its internal state.
@hhvm-bot hhvm-bot closed this in 7cc39aa Oct 20, 2015
jwatzman pushed a commit that referenced this pull request Oct 21, 2015
Summary: When the input string is shorter than the shortest input
pattern the last position to visit is set to -1. Patch
short circuits and returns input string when all patterns
are longer than the input string.
Closes #6071

Reviewed By: jwatzman

Differential Revision: D2367627

fb-gh-sync-id: 704ab038e68145916460c890bf6522dd62910af8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants