socketry / nio4r

Cross-platform asynchronous I/O primitives for scalable network clients and servers.
Other
965 stars 86 forks source link

avoid possible junk data in `monitor` through memory initialization #248

Closed boazsegev closed 3 years ago

boazsegev commented 3 years ago

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 commented 3 years ago

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

tarcieri commented 3 years ago

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.

boazsegev commented 3 years ago

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 commented 3 years ago

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.

tarcieri commented 3 years ago

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

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

boazsegev commented 3 years ago

FYI:

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

tarcieri commented 3 years ago

Cool!

ioquatix commented 3 years ago

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