Fixed "invalid HTTP upgrade" issue

[psahgal]

I was getting occasional errors from my app stating "invalid HTTP upgrade". It sounds like a few other users of the library were also seeing this issue. I tracked down the problem: there were some extra garbage bytes at the beginning of the HTTP response from the server. These garbage bytes come out of NSStream, and are identified as a leading fragment for the response packet. The extra bytes didn't show up when I was inspecting the packets using Wireshark.

To fix the issue, I added some code in processHTTP to look for the start of the response, and ignore any bytes before the response.

I should also note that I was having trouble with running the tests, as stated in issue #297. Let me know if there's any test failures.

@daltoniam Please review.

[acmacalister]

@Elethier do you happen to know what these "garbage" bytes were and where they are coming from? Seems peculiar the server would send that in it's HTTP response randomly and it would be nice to know what exactly those bytes are, before just ignoring them. Do have a way to reproduce the behavior? Love to look at it on my end as well.

[psahgal]

@acmacalister I'll have to spend some time to reproduce the issue, as we've had this fix in Swift 2.2 for a while, and I finally had time to migrate it. Here's some information about what I was seeing at the time:

• I was running Wireshark to diagnose connection problems in our app, and I was able to see valid HTTP upgrade requests, along with a properly formatted 101 response. But our app was throwing the "Invalid HTTP Upgrade" error instead.
• I stepped through the code handling the response in Starscream and I found that there was an extra few characters at the start of the response. As a result, processHTTP was returning an error code instead of a 101.
• Those extra few characters were coming from the InputStream through the stream function, as a 6-byte fragment before the bulk of the response.
• Starscream took this extra 6 byte fragment, assumed the response was divided up into 2 packets, and appended the 6 bytes to the next fragment received. The input to processHTTP ended up being something like &%HTTP.... The start of the string was incorrect.

I am still unsure if the extra 6 bytes was a server-side problem, but the Wireshark packet capture seems to suggest it's a client-side issue. What's weird is that I was clearly seeing a 6-byte fragment coming out of Apple's NSStream API when it shouldn't have. The only conclusion I can come up with is that there's a bug in Apple's code somewhere.

Also, it looks like this problem may have shown up in another person's app, since a Github issue was opened about it.

Sadly, I do not have a good way to reproduce the behavior, as our app's server is a piece of hardware running custom firmware. I'm not sure I can give you any good information on what it's doing, unfortunately.

[markquezada]

Just as an FYI, we're also seeing this behavior. We're using starscream with an elixir/phoenix server via PhoenixSwiftClient in case that helps.

[markquezada]

@Elethier @acmacalister FYI, we tried using the above fix and it resolved the issue for us. Any thoughts on getting this merged in?

I agree that it'd be nice to know why this is happening before blindly fixing it, but I'm not sure there are any adverse affects of having this in in the meantime.

[markquezada]

Woops, should have pinged @daltoniam I think.

[acmacalister]

@Elethier @markquezada I appreciate the info. I understand the desire to merge this, but here is my line of thinking. Just ignoring the payload up the point of looking for the start of an HTTP string isn't standard HTTP processing behavior and not something other WebSocket clients do. Looking for the standard carriage return \r\n\r\n like we currently do is the standard. I agree this probably won't cause an issue for anyone, but I don't think pushing this out to all the users of Starscream as a patch is the right call. It seems more like a bandaid to a larger problem. I generally don't assume issues in Foundation API like NSStream. While possible, they are generally less likely and since other WebSocket libraries in both Swift and Objc don't do this, it make me lean toward an issue in Starscream as we have seen the issue on different server implementations as well. I'm leaning toward an issue with how we process the inputStream and not properly resetting memory. I'm going to take a look at that on my end and see I can reproduce the issue as time permits. If either of you want to test my theory since you are currently seeing the issue, try and add this:

buf!.setData(Data())

right after this line:

https://github.com/daltoniam/Starscream/blob/master/Source/WebSocket.swift#L454

My theory is that the memory is being reallocated over the top of old memory and is not zero filled, leading toward those useless bytes showing up in the input stream.

[markquezada]

@acmacalister Ah, interesting. If your theory is true, that could be a pretty bad bug right?

Totally agree with your assessment. We'll try investigating a bit more and report back. Thanks for the info!

[acmacalister]

Yeah, it would certainly be a bug. A fairly common one when working in language that have unsafe memory (aka C). The thought process had been that the MutableData is zero filled on initialization since Swift is a higher level language, but that might an incorrect assumption. Not sure there is really anyway to know for sure since Foundation is closed source and I don't believe the docs speak to it at all.

[psahgal]

@acmacalister I like this plan, it would make much more sense that it's a bug within the socket library. I'll investigate and let you know if your proposed change fixes the issues we're seeing.

[markquezada]

@acmacalister Just as an FYI, we tried adding the line you mentioned but it didn't seem to have any effect. We're still seeing the "invalid HTTP upgrade" error. @Elethier did you have any luck?

[psahgal]

@markquezada @acmacalister I'm still seeing the error show up, unfortunately. @acmacalister you meant adding buf!.setData(Data()) after line 463, right? It didn't make too much sense to put it in cleanupStream(). I tried buf!.setData(Data()) and buf!.resetByes(NSMakeRange(0, BUFFER_MAX), and neither of them worked.

But the idea of an NSData object not being cleared might still be valid. I haven't looked at it too much, but what happens to the fragBuffer object on a disconnect? Is it cleared? Because the garbage bytes I was seeing were always within the fragBuffer. Maybe adding fragBuffer = nil on disconnect will fix the problem.

I'll have to wait until tomorrow to test it, though.

[nuclearace]

I didn't think the fragBuffer is used in processHTTP? How are the garbage bytes there?

You could see if replacing these lines with

let buffer = calloc(BUFFER_MAX, MemoryLayout<UInt8>.size).assumingMemoryBound(to: UInt8.self)

if you're concerned about unzero'd memory, but the read should be overwriting any garbage bytes at the start of the buffer anyway, I would think. Also the documentation for NSMutableData does not say calling init(capacity:) zeros anything, however calling init(length:) does.

Also open source Foundation implementation for NSMutableData just ignores the capacity parameter.

[psahgal]

@nuclearace The dequeueInput() function uses fragBuffer here. If the socket isn't connected, it then calls processTCPHandshake(), which calls processHTTP(). My theory is that if fragBuffer isn't cleared when the socket is disconnected, the last fragment received will be appended to the start of the first response received by the socket when it reconnects. I don't see fragBuffer being cleared on disconnect, so it's possible there's garbage data lying around.

[nuclearace]

Ah yeah, if that frag buffer isn't cleared it might contain some stale data. I never reuse the same socket so I wouldn't have run into this issue most likely.

[daltoniam]

I checked in some cleanup stuff to master. I think it might fix this but clearing the fragBuffer. Let me know if that works and we will cut a new version with the fix!

[psahgal]

@daltoniam Looks like your change fixed the issues on my end. @markquezada, did you try the fix on your end?

[markquezada]

@Elethier Looks like we're still experiencing this issue, even with the code from master. Not sure what to try next.

[psahgal]

@markquezada When I have some time, I'll retest the fix. We only see the issue sporadically, so maybe I got lucky.

[psahgal]

@markquezada Just retested today, I'm not seeing "Invalid HTTP Upgrade" with the code on the master branch. How are you testing this issue? I put a breakpoint on line 545 in Websocket.swift: doDisconnect(errorWithDetail("Invalid HTTP upgrade", code: UInt16(code))). Are you seeing your code hit this breakpoint?

[markquezada]

@Elethier sorry about the delay. We ended up doing more testing and found out that the issue does seem to be fixed. We do still get an "Invalid HTTP Upgrade" message but we realized that it's occurring at the proper time in response to a 403 error code when attempting to join a channel. (Unrelated to the bug listed here.)

@daltoniam 👍 from me to ship a release. Looks like the work you did in master fixed this for us. Thank you!

[psahgal]

@markquezada Good to hear that the changes fixed your problems, too.

@daltoniam Thanks for fixing this in the master branch. I'll close this pull request.

[qadirsuh]

Hi guys. I updated my code from NSURL to NSURLComponents I am started getting the same error i.e invalid HTTP upgrade and sometimes The operation couldn’t be completed. (OSStatus error -9806.)

Here was my code using NSURL

            socket = WebSocket(url: (NSURL(scheme: "ws", host: "192.168.14.247:8888/ws/", path: "/") as? URL)!)
            socket!.headers["Upgrade"] = "WebSocket"
            socket!.headers["Connection"] = "Upgrade"
            socket!.headers["Sec-WebSocket-Protocol"] = hash as String
            
            socket!.delegate = self
            socket!.connect()

now I am using code below

            let websocketScheme = "wss" //also tried with "ws"
            let host = "192.168.14.247"
            let port: NSNumber = 8888
            
            let components = NSURLComponents()
            components.scheme = websocketScheme
            components.host = host
            components.port = port
            components.path = "/"
            
            print("URL \(components.url)")
            
            socket = WebSocket(url: components.url!)
            
            socket!.headers["Upgrade"] = "WebSocket"
            socket!.headers["Connection"] = "Upgrade"
            socket!.headers["Sec-WebSocket-Protocol"] = hash as String
            
            socket!.delegate = self
            socket!.connect()

I have integrated the library using pods. is this issue fixed in pods too.
I can't see any update of starscream on pods. (using command => pod outdated in terminal)

https://www.dropbox.com/s/wp6cf0ftl8cmjtk/Screenshot%202017-02-02%2019.17.33.png?dl=0

[psahgal]

@qadirsuh The fix for this issue has not made it into a release yet. Can you confirm that you have the changes in this commit in your local version of Starscream? If you're using version 2.0.2, I don't think this change will be in your project.

Perhaps now would be a good time to issue another release...

[qadirsuh]

@Elethier yes I am using 2.0.2. just confirmed it from pod.lock file

- Starscream (2.0.2) with SPEC CHECKSUM 6c135a34e0a6e60cedaa0b30db67a4c05cf7cd38

Here is my pod.lock file

  - AFNetworking (3.1.0):
    - AFNetworking/NSURLSession (= 3.1.0)
    - AFNetworking/Reachability (= 3.1.0)
    - AFNetworking/Security (= 3.1.0)
    - AFNetworking/Serialization (= 3.1.0)
    - AFNetworking/UIKit (= 3.1.0)
  - AFNetworking/NSURLSession (3.1.0):
    - AFNetworking/Reachability
    - AFNetworking/Security
    - AFNetworking/Serialization
  - AFNetworking/Reachability (3.1.0)
  - AFNetworking/Security (3.1.0)
  - AFNetworking/Serialization (3.1.0)
  - AFNetworking/UIKit (3.1.0):
    - AFNetworking/NSURLSession
  - Alamofire (4.2.0)
  - AlamofireImage (3.2.0):
    - Alamofire (~> 4.1)
  - Fabric (1.6.11)
  - Firebase/Auth (3.11.0):
    - Firebase/Core
    - FirebaseAuth (= 3.1.0)
  - Firebase/Core (3.11.0):
    - FirebaseAnalytics (= 3.6.0)
    - FirebaseCore (= 3.4.6)
  - Firebase/Crash (3.11.0):
    - Firebase/Core
    - FirebaseCrash (= 1.1.4)
  - Firebase/Database (3.11.0):
    - Firebase/Core
    - FirebaseDatabase (= 3.1.1)
  - FirebaseAnalytics (3.6.0):
    - FirebaseCore (~> 3.4)
    - FirebaseInstanceID (~> 1.0)
    - GoogleInterchangeUtilities (~> 1.2)
    - GoogleSymbolUtilities (~> 1.1)
    - GoogleToolboxForMac/NSData+zlib (~> 2.1)
  - FirebaseAuth (3.1.0):
    - FirebaseAnalytics (~> 3.6)
    - GoogleToolboxForMac/NSDictionary+URLArguments (~> 2.1)
    - GTMSessionFetcher/Core (~> 1.1)
  - FirebaseCore (3.4.6):
    - GoogleInterchangeUtilities (~> 1.2)
    - GoogleToolboxForMac/NSData+zlib (~> 2.1)
  - FirebaseCrash (1.1.4):
    - FirebaseAnalytics (~> 3.5)
    - FirebaseInstanceID (~> 1.0)
    - GoogleInterchangeUtilities (~> 1.2)
    - GoogleSymbolUtilities (~> 1.1)
    - GoogleToolboxForMac/Logger (~> 2.1)
    - GoogleToolboxForMac/NSData+zlib (~> 2.1)
  - FirebaseDatabase (3.1.1):
    - FirebaseAnalytics (~> 3.5)
  - FirebaseInstanceID (1.0.8)
  - Flurry-iOS-SDK/FlurryAds (7.1.1):
    - Flurry-iOS-SDK/FlurrySDK
  - Flurry-iOS-SDK/FlurrySDK (7.1.1)
  - ForecastIOClient (0.2.1):
    - AFNetworking
    - SwiftyJSON
  - GoogleInterchangeUtilities (1.2.2):
    - GoogleSymbolUtilities (~> 1.1)
  - GoogleSymbolUtilities (1.1.2)
  - GoogleToolboxForMac/DebugUtils (2.1.0):
    - GoogleToolboxForMac/Defines (= 2.1.0)
  - GoogleToolboxForMac/Defines (2.1.0)
  - GoogleToolboxForMac/Logger (2.1.0):
    - GoogleToolboxForMac/Defines (= 2.1.0)
  - GoogleToolboxForMac/NSData+zlib (2.1.0):
    - GoogleToolboxForMac/Defines (= 2.1.0)
  - GoogleToolboxForMac/NSDictionary+URLArguments (2.1.0):
    - GoogleToolboxForMac/DebugUtils (= 2.1.0)
    - GoogleToolboxForMac/Defines (= 2.1.0)
    - GoogleToolboxForMac/NSString+URLArguments (= 2.1.0)
  - GoogleToolboxForMac/NSString+URLArguments (2.1.0)
  - GTMSessionFetcher/Core (1.1.7)
  - Kingfisher (3.2.4)
  - p2.OAuth2 (3.0.1)
  - Smooch (5.3.0)
  - Starscream (2.0.2)
  - SwiftyJSON (3.1.3)

DEPENDENCIES:
  - Alamofire (~> 4.0)
  - AlamofireImage
  - Fabric
  - Firebase/Auth
  - Firebase/Core
  - Firebase/Crash
  - Firebase/Database
  - Flurry-iOS-SDK/FlurryAds (~> 7.1.1)
  - Flurry-iOS-SDK/FlurrySDK (~> 7.1.1)
  - ForecastIOClient
  - Kingfisher (~> 3.0)
  - p2.OAuth2 (~> 3.0)
  - Smooch
  - Starscream (~> 2.0.0)

SPEC CHECKSUMS:
  AFNetworking: 5e0e199f73d8626b11e79750991f5d173d1f8b67
  Alamofire: aa2e09d871c9160ac53c90e83c68064a94e3dfbe
  AlamofireImage: 157ed682cc81d3b9db4fb90c1f12180ac552d93b
  Fabric: 5911403591946b8228ab1c51d98f1d7137e863c6
  Firebase: b8134e285eb33201115bc688bc5d7f112923bc1f
  FirebaseAnalytics: 9c67af0ebeb8d2146c9b4ea2616439affa947b58
  FirebaseAuth: b436e4ecebe068bf8e8c6e4e29de5756ffe7ee24
  FirebaseCore: 03da1cb32615569bbc2830a22f9ad753d9a02ef5
  FirebaseCrash: 34f0349908ec87945a5ae96c402e7201a3f3aede
  FirebaseDatabase: 6de92187481ff0e8e124064f21742e95ee883b02
  FirebaseInstanceID: ba1e640935235e5fac39dfa816fe7660e72e1a8a
  Flurry-iOS-SDK: 83fd4f40ae66e80d122fd2f6698f0e0c31021a63
  ForecastIOClient: b1c9d63382ffc816bcbe391c4c9815ecc0c5df03
  GoogleInterchangeUtilities: d5bc4d88d5b661ab72f9d70c58d02ca8c27ad1f7
  GoogleSymbolUtilities: 631ee17048aa5e9ab133470d768ea997a5ef9b96
  GoogleToolboxForMac: 2b2596cbb7186865e98cadf2b1e262d851c2b168
  GTMSessionFetcher: a1f8ed39e4fe21c68957daed472c7afbcdf29166
  Kingfisher: 8d80f39da403cd9c9ee11984e1655f4d6a566cdb
  p2.OAuth2: 1c58296b53820e57ea63a8977541d2e4780df141
  Smooch: ad6bc010f8ed2bc0cdca7421fd999122923084b2
  Starscream: 6c135a34e0a6e60cedaa0b30db67a4c05cf7cd38
  SwiftyJSON: 38a8ea2006779c0fc4c310cb2ee8195327740faf

PODFILE CHECKSUM: 385e4820ca379a8456e15c73b96f373c1c12b7b8

COCOAPODS: 1.0.1

[psahgal]

@qadirsuh As I said, the changes to fix "invalid HTTP upgrade" aren't in 2.0.2, they're just on master. I recommend you replace this line in your Podfile:

pod 'Starscream', '2.0.2'

with this:

pod 'Starscream', :git => https://github.com/daltoniam/Starscream.git', :commit => '60b27a4388bcb1b8b170c645dd19142cfa674f59'

This will let you use the pre-release version. But use this version with caution; it's better to use a release tag when you can.

[qadirsuh]

Thanks @Elethier for pointing this, I have changed my pod as you suggested above. I am still experiencing the same error "Invalid HTTP upgrade" :(

• Starscream (from https://github.com/daltoniam/Starscream.git, commit 60b27a4388bcb1b8b170c645dd19142cfa674f59)

pod.lock file now showing like below.

EXTERNAL SOURCES:
  Starscream:
    :commit: 60b27a4388bcb1b8b170c645dd19142cfa674f59
    :git: https://github.com/daltoniam/Starscream.git

CHECKOUT OPTIONS:
  Starscream:
    :commit: 60b27a4388bcb1b8b170c645dd19142cfa674f59
    :git: https://github.com/daltoniam/Starscream.git

did I miss any thing still?