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

SR-5557: Fix NSRegularExpression errors with invalid pattern #1164

Merged
merged 1 commit into from Aug 11, 2017

Conversation

spevans
Copy link
Collaborator

@spevans spevans commented Aug 9, 2017

  • The error dictionary in CFRegularExpression was failing with the
    error key being a CFSTR inside an array so initialiase it outside
    of the array.

I removed the _nsObject as I think throwing the error dictionary is more useful as it gave a better error message when stringified.

Should a description be added to the error dictionary to eliminate the '(null)'?

@ianpartridge
Copy link
Collaborator

I probably haven't had enough coffee but I don't quite follow this one... Why does it make a difference whether it's an array of CFStringRef or a CFStringRef inside an array of CFTypeRef?

@spevans
Copy link
Collaborator Author

spevans commented Aug 9, 2017

When CFSTR("NSInvalidValue") was declared directly inside the array it would crash CFHash, which is the actual fix.

I don't think the CFStringRef v CFTypeRef makes much difference here except that CFTypeRef seems to be the correct type for a key based on other code I looked at.

I think we will need either @parkera or @phausler to validate this fix.

- The error dictionary in CFRegularExpression was failing with the
  error key being a CFSTR inside an array so initialiase it outside
  of the array.
@ianpartridge
Copy link
Collaborator

Yes you're right. I imagine this code is unchanged from Darwin so maybe there's some other factors involved.

@spevans
Copy link
Collaborator Author

spevans commented Aug 9, 2017

Yes, although it did actually crash corelibs-foundation on macOS as well, which makes even less sense!

@phausler
Copy link
Member

phausler commented Aug 9, 2017

The problem previously with the key was because CFSTR in swift-corelibs does not use the builtin but instead uses an inline address of. That hack won't work with c arrays.

Overall this change looks reasonable to me.

@spevans
Copy link
Collaborator Author

spevans commented Aug 9, 2017

Thats good to know. I did a quick audit of CSTR in CoreFoundation and noticed a few places where it is used in arrays so I'll add a PR soon to fix those cases

@ianpartridge
Copy link
Collaborator

@swift-ci please test and merge

@spevans
Copy link
Collaborator Author

spevans commented Aug 11, 2017

@ianpartridge This looks to have failed due to an unrelated swift test failing, can you retry?

@ianpartridge
Copy link
Collaborator

@swift-ci please test and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants