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

Validate data before calling NetService.dictionary #86

Merged
merged 2 commits into from
Apr 4, 2020

Conversation

digaus
Copy link
Contributor

@digaus digaus commented Jan 17, 2020

This prevents the crash of the app when a broadcast has the wrong format

#85
#79
#32

Decode the TXT record as a string dictionary, or [:] if the data is malformed: https://stackoverflow.com/a/52220739
Remove import
@digaus digaus changed the title PReven Prevent iOS from crashing when calling NetService.dictionary on malformed data Jan 17, 2020
@digaus digaus changed the title Prevent iOS from crashing when calling NetService.dictionary on malformed data Validate data before calling NetService.dictionary Jan 17, 2020
@digaus digaus requested a review from emcniece January 17, 2020 18:42
@emcniece
Copy link
Collaborator

Thank you for hunting this down!

What iOS versions have you tested this on? I'm not sure exactly how far back we need (or want) to support, but it would be good to know that it works on latest - 1 at least.

Any chance you have a link to the txtRecordData spec handy? How has the record format changed in such a way that breaks the original parsing?

@digaus
Copy link
Contributor Author

digaus commented Jan 17, 2020

Thanks for the fast response :) I tested it on 13.3 iPadOS and a user successfully tested on 13.3 iOS.

Have no other devices on hand to do more testing. The code I use is from here:

https://stackoverflow.com/a/52220739

Another solution would be to use the Objective C call:

https://stackoverflow.com/a/41371961

Copy link
Collaborator

@emcniece emcniece left a comment

Choose a reason for hiding this comment

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

Got a few questions around the new dictionary method. Please forgive my lack of Swift-fu 🙇

var result = [String: String]()
var data = txtData

while !data.isEmpty {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to get stuck inside this while? If there is a timeout, how long does it take? We have to be aware of (and prevent if possible) long-running Cordova requests.

case 2:
result[key] = String(keyValue[1])
default:
fatalError()
Copy link
Collaborator

Choose a reason for hiding this comment

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

How "fatal" is this - does it stackdump the entire application, or will Cordova pass this back as a failed promise? If it does dump, what do you think about catching and returning to Cordova with a failure message so the developer can handle it?


while !data.isEmpty {
// The first byte of each record is its length, so prefix that much data
let recordLength = Int(data.removeFirst())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably covered by the spec or data structure, but just in case: it's not possible for the length to be greater than one byte, right?

let recordData = data[..<(data.startIndex + recordLength)]
data = data.dropFirst(recordLength)

guard let record = String(bytes: recordData, encoding: .utf8) else { return [:] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we now have 3 possible return values from this method:

  1. Empty key-value ([:])
  2. Populated key, but empty value (case 1: result[key] = "")
  3. Populated key and value (case 2: result[key] = String(keyValue[1]))

Previously I think if there were no records at all then the final JSON response looks like "txtRecord": {}. What does it look like now in the event that one of these guards returns [:]?

These guards might be a good spot to do some debug printing too.

@digaus
Copy link
Contributor Author

digaus commented Jan 17, 2020

@emcniece

Valid questions but I cannot answer any of that, I have no experience in swift and just searched the issue until I found someone who ran into that aswell.

You might find a better solution since we now know why the crash happens. I can do some testing on what responses I get for the 'textRecord'.

Might also try this possible fix: https://stackoverflow.com/a/40473424

It only checks for correctness and then calls the initial method.

@emcniece
Copy link
Collaborator

emcniece commented Apr 4, 2020

Tested, debugged, and approved. I'd like to add some text to the fatalError() call but I'll do that in the next PR.

Thank you so much for helping us fix this!

@emcniece emcniece self-requested a review April 4, 2020 20:10
Copy link
Collaborator

@emcniece emcniece left a comment

Choose a reason for hiding this comment

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

🚀

@emcniece emcniece merged commit 27cdb18 into becvert:master Apr 4, 2020
@emcniece emcniece mentioned this pull request Apr 4, 2020
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.

2 participants