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

avoid possible junk data in monitor through memory initialization #248

Merged
merged 5 commits into from
Sep 15, 2020
Merged

avoid possible junk data in monitor through memory initialization #248

merged 5 commits into from
Sep 15, 2020

Conversation

boazsegev
Copy link
Contributor

This short PR explores the possibility of adding a slightly redundant if statement...

The if statement is probably redundant since rb_gc_mark tests for special non-allocated "objects" such as nil, false, embedded numbers etc').

However: (1) since a NULL dereferencing issue was reported; and (2) GC tests this condition only after pushing the stack (so it's faster if we test for it); we might as well add this slightly redundant if statement and see if it helps.

This short PR explores the possibility of adding a slightly redundant `if` statement...

The if statement is probably redundant since `rb_gc_mark` tests for special non-allocated "objects" such as `nil`, `false`, embedded numbers etc').

However: (1) since a NULL dereferencing issue was reported; and (2) GC tests this condition only after pushing the stack (so it's faster if we test for it); we might as well add this slightly redundant `if` statement and see if it helps.
@boazsegev
Copy link
Contributor Author

See comments in #244 as to why I propose this extra if statement

@tarcieri
Copy link
Contributor

tarcieri commented Sep 15, 2020

Note that monitor->self is never explicitly set to NULL, so this change alone doesn't make sense.

It's initialized in NIO_Monitor_initialize:

https://github.com/socketry/nio4r/blob/77f859a/ext/nio4r/monitor.c#L107

Prior to that it's uninitialized. If you'd like to add this NULL check, I would suggest initializing monitor->self to NULL in NIO_Monitor_allocate.

This commit addresses the comment made by @tarcieri about the `monitor` object's memory not being initialized.
While allocating the object, we might as well initialize the whole memory block to zero in order to protect against hidden issues.
@boazsegev
Copy link
Contributor Author

Thanks @tarcieri - I added code to validate that the allocation was successful and to initialize the memory.

I am in doubt if this was the issue, but these validations make for cleaner and safer code. For example, allocation failure should have been tested for and memory initialization is a good practice that protects against some hard to catch bugs.

@boazsegev
Copy link
Contributor Author

I get a failure:

In file included from nio4r_ext.c:6:
./../libev/ev.c:54:12: fatal error: 'config.h' file not found
#  include "config.h"

This isn't me... I didn't touch that.

ext/nio4r/monitor.c Outdated Show resolved Hide resolved
ext/nio4r/monitor.c Outdated Show resolved Hide resolved
@tarcieri
Copy link
Contributor

This isn't me... I didn't touch that.

Not sure what's up with that. Any ideas @ioquatix?

Should resolve the review offered by @tarcieri .
Co-authored-by: Tony Arcieri <bascule@gmail.com>
@boazsegev boazsegev changed the title Add if statement, even if redundant avoid possible junk data in monitor through memory initialization Sep 15, 2020
@boazsegev
Copy link
Contributor Author

FYI:

I tested the PR on my machine and I think it solved the issue that was commented upon in PR #244

@tarcieri
Copy link
Contributor

Cool!

@ioquatix
Copy link
Member

Thanks everyone for figuring this out. I'll do another point release.

@boazsegev boazsegev deleted the patch-1 branch September 15, 2020 21:21
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.

3 participants