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

fix and improve stdio error handling #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

N-R-K
Copy link

@N-R-K N-R-K commented May 27, 2023

the IO error checking currently done via the *_noerr functions are entirely misguided. stdio functions are typically buffered, so most of these calls are more or less just memcpy. and since the password isn't likely going to be big enough to fill the buffer to cause a flush (i,e an actual IO operation) these calls are almost never going to fail - with the exception of the newline character - which might cause a flush if stdout is "interactive" and thus set to line-buffered mode.

but there's no guarantee stdout is interactive, user might want to pipe the output to some encryption tool for example. in such cases, the stdout flush will occur due to calling exit() and any errors that occur at that point will be entirely missed by the application.

the proper way to error check stdio functions is to manually flush at the end and check for the error flag on the stream. here's the result before and after the patch:

[dictpw master]~> ./build/dictpw >/dev/full
[dictpw io_err]~> ./build/dictpw >/dev/full
dictpw: Failed to write to stdout

additionally, the way *_noerr functions are checking for errno is also confusing:

  • if the plan is to support only POSIX systems, then POSIX specifies fputs and fputc to set the errno on failure. so the errno != 0 check is entirely redundant.
  • and if non-POSIX systems are to be supported, then the check is incorrect since ISO C doesn't require errno to be set for these functions on failure. so if such a system exists, the application will see that fputs failed but errno is 0 and thus will march on thinking everything went fine.

one regression from this patch is that the exact cause of error is not known anymore. so the error message will be the same for any errors.

the IO error checking currently done via the `*_noerr` functions are
entirely misguided. stdio functions are typically buffered, so most of
these calls are more or less just memcpy. and since the password isn't
likely going to be big enough to fill the buffer to cause a flush (i,e
an *actual* IO operation) these calls are almost never going to fail -
with the exception of the newline character - which might cause a flush
if stdout is "interactive" and thus set to line-buffered mode.

but there's no guarantee stdout is interactive, user might want to pipe
the output to some encryption tool for example. in such cases, the
stdout flush will occur due to calling `exit()` and any errors that
occur at that point will be entirely missed by the application.

the proper way to error check stdio functions is to manually flush at
the end and check for the error flag on the stream. here's the result
before and after the patch:

	[dictpw master]~> ./build/dictpw >/dev/full
	[dictpw io_err]~> ./build/dictpw >/dev/full
	dictpw: Failed to write to stdout

additionally, the way `*_noerr` functions are checking for `errno` is
also confusing:

* if the plan is to support only POSIX systems, then POSIX specifies
  fputs and fputc to set the errno on failure. so the `errno != 0` check
  is entirely redundant.
* and if non-POSIX systems are to be supported, then the check is
  *incorrect* since ISO C doesn't require errno to be set for these
  functions on failure. so if such a system exists, the application will
  see that fputs failed but errno is 0 and thus will march on thinking
  everything went fine.

one regression from this patch is that the exact cause of error is not
known anymore. so the error message will be the same for any errors.
@N-R-K
Copy link
Author

N-R-K commented May 27, 2023

A couple more notes: this is just one way to deal with it. You could also:

  • Switch to raw write(2) syscalls
  • flush after each call. E.g
if (fputs(str, stdout) == EOF || fflush(stdout) == EOF)
        errx(1, "writing to stdout failed");
  • You could also set the stream to be unbuffered, but I don't know how well that'll work. I'm aware of at least one case where unbuffered stream resulted in one-byte-at-a-time operation with a syscall for each byte.

One more thing, keep in mind that library routines are allowed to modify errno even on success. So setting errno = 0 and then calling a library routine might not really do what you might expect.

Finally, I'm not a user of this program, just happened to notice some potential improvement and thus opening the PR. I don't have much desire to work on this patch further, feel free to modify it or take the other routes suggested above.

@guijan
Copy link
Owner

guijan commented Jun 17, 2023

Thank you for this PR, I will switch to write().

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.

2 participants