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

Add checks to the conversion of Unix file descriptors to I/O channels #1825

Merged
merged 5 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@xavierleroy
Copy link
Contributor

commented Jun 8, 2018

This GPR adds checks to the Unix.{in,out}_channel_of_descr functions so that they fail if the given file descriptor is not suitable for character-oriented I/O, e.g. a block device or a datagram socket.

Without such checks, the I/O channel is created, but will most likely fail in mysterious ways when flushed or refilled.

This addresses part of MPR#7238

xavierleroy added some commits Oct 9, 2017

Unix.{in,out}_channel_of_descr: check that descr has stream semantics
Have these functions fail if the given file descriptor is not suitable for
character-oriented I/O, e.g. a block device or a datagram socket.
Part of MPR#7238.

This is WIP.  Needs porting to Windows and testing.
Unix.{in,out}_channel_of_descr: check that descr has stream semantics
Added Win32 implementation.
Changed Unix implementation to report EINVAL in case of non-stream semantics.
struct stat buf;

if (fstat(fd, &buf) == -1) return errno;
switch (buf.st_mode & S_IFMT) {

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 8, 2018

Member

I think this is sufficient, but someone who can read POSIXese will have to say if one of these is set when the various S_TYPE macros for memory queues and semaphores and the like are true. It may be that one has to check for that separately?

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 8, 2018

Member

Actually, thinking about this, if none of those bits are set, the default: case will trigger anyway.

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 8, 2018

Author Contributor

As far as I know, memory queues and semaphores cannot be opened as file descriptors, e.g. sem_open returns a sem_t *, and is not made available by the OCaml Unix library.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 8, 2018

Member

So my comment was inapplicable two distinct ways.

return 0;
default: {
DWORD err = GetLastError();
return err == 0 ? ERROR_INVALID_ACCESS : err;

This comment has been minimized.

Copy link
@diml

diml Jun 12, 2018

Member

I would write NO_ERROR here, which is slightly clearer than 0

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 13, 2018

Author Contributor

Nice suggestion, implemented in 0724940

@diml

diml approved these changes Jun 12, 2018

Copy link
Member

left a comment

I read the code and it looks correct to me. This check seems useful to me.

xavierleroy added some commits Jun 13, 2018

Unix.{in,out}_channel_of_descr: check that descr has stream semantics
Use `NO_ERROR` instead of `0` to mean "no error occurs".

@xavierleroy xavierleroy merged commit ea9dc4e into ocaml:trunk Jun 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xavierleroy xavierleroy deleted the xavierleroy:check-stream-semantics branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.