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

Missing remoteAddress in ChatServer (Crash) #97

Closed
ezfe opened this issue Mar 4, 2018 · 6 comments
Closed

Missing remoteAddress in ChatServer (Crash) #97

ezfe opened this issue Mar 4, 2018 · 6 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ezfe
Copy link

ezfe commented Mar 4, 2018

Expected behavior

When the ChatClient sends a message, other clients see

(##remoteAddress##) - ##message##

Actual behavior

The ChatServer force-unwraps the remoteAddress field, encountering nil, and crashes.

Steps to reproduce

  1. Run ChatServer
  2. Run ChatClient (once is all that's needed to demonstrate crash, twice for full set up)
  3. Send message by typing in ChatClient and pressing enter
  4. ChatServer unexpectedly encounters nil here:
    buffer.write(string: "(\(ctx.channel.remoteAddress!)) - ")

Workaround

Modifying the above line referenced in part 4 to the following produces what appears to be expected behavior. However, I am unclear as to the difference between ctx.channel.remoteAddress and ctx.remoteAddress.

if let remoteAddress = ctx.remoteAddress {
    buffer.write(string: "(\(remoteAddress)) - ")
} else {
    buffer.write(string: "(no address) - ")
}

If possible, minimal yet complete reproducer code (or URL to code)

Provided ChatServer and ChatClient exhibit this behavior

SwiftNIO version/commit hash

2c1d993 (latest as this issue)

Swift & OS version (output of swift --version && uname -a)

Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)
Target: x86_64-apple-macosx10.9
Darwin Ezekiels-MacBook-Pro-133.local 17.5.0 Darwin Kernel Version 17.5.0: Sun Feb 11 23:54:24 PST 2018; root:xnu-4570.50.279~25/RELEASE_X86_64 x86_64

@ezfe ezfe changed the title Missing remoteAddress Missing remoteAddress in ChatServer Mar 4, 2018
@ezfe ezfe changed the title Missing remoteAddress in ChatServer Missing remoteAddress in ChatServer (Crash) Mar 4, 2018
@normanmaurer
Copy link
Member

normanmaurer commented Mar 4, 2018 via email

@normanmaurer
Copy link
Member

@ezfe hmm I can not reproduce it :( The remoteAddress should never be nil when channelRead is called.

@normanmaurer
Copy link
Member

@ezfe ah never mind... I did execute the wrong example :(

normanmaurer added a commit to normanmaurer/swift-nio that referenced this issue Mar 4, 2018
Motivation:

We missed to correctly update the cached remote and local addresses for accepted Channels. Because of this localAddress and remoteAddr always returned nil.

Modifications:

- Update cached addresses when constructing SocketChannel from existing Socket.
- Add testcase

Result:

Fixes [apple#97].
@normanmaurer
Copy link
Member

@ezfe this should be fixed by #98 ... Please check

@normanmaurer normanmaurer self-assigned this Mar 4, 2018
@normanmaurer normanmaurer added the bug Something isn't working label Mar 4, 2018
@ezfe
Copy link
Author

ezfe commented Mar 4, 2018

@normanmaurer I've confirmed that this resolves the issue. You may close this issue when appropriate.

Lukasa pushed a commit that referenced this issue Mar 5, 2018
Motivation:

We missed to correctly update the cached remote and local addresses for accepted Channels. Because of this localAddress and remoteAddr always returned nil.

Modifications:

- Update cached addresses when constructing SocketChannel from existing Socket.
- Add testcase

Result:

Fixes [#97].
@Lukasa
Copy link
Contributor

Lukasa commented Mar 5, 2018

Resolved by #98.

@Lukasa Lukasa closed this as completed Mar 5, 2018
@normanmaurer normanmaurer added this to the 1.1.0 milestone Mar 5, 2018
weissi pushed a commit to weissi/swift-nio that referenced this issue Jun 13, 2020
Motivation:

When we're validating that settings are in their bounds, we should probably endeavour to
express those bounds correctly.

Modifications:

- Fixed the bounds checks on SETTINGS_INITIAL_WINDOW_SIZE and SETTINGS_MAX_FRAME_SIZE.
- Added regression tests.

Result:

Better correctness.
weissi pushed a commit to weissi/swift-nio that referenced this issue Feb 3, 2024
* Wrap in parentheses

* Revert previous changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants