4 ms·

> I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not option

by im3w1l 6y ago

> I was a TA in a first-semester course in C programming for several years, and one of the harder things to inculcate is how return value checking is not optional. It's very tempting to assume your standard library function just works, always. The situation is worse than that. People check for errors except when they don't. And you are expected to know the difference. Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done. Or take close(2). > If close() is interrupted by a signal that is to be caught, it shall return -1 with errno set to EINTR and the state of fildes is unspecified. > This permits the behavior that occurs on Linux and many other implementations, where, as with other errors that may be reported by close(), the file descriptor is guaranteed to be closed. However, it also permits another possibility: that the implementation returns an EINTR error and keeps the file descriptor open. (According to its documentation, HP-UX's close() does this.) The caller must then once more use close() to close the file descriptor, to avoid file descriptor leaks. This divergence in implementation behaviors provides a difficult hurdle for portable applications, since on many implementations, close() must not be called again after an EINTR error, and on at least one, close() must be called again. How did this discrepancy go unnoticed for so long? Because no one checks close return value. Everyone nods and say you should, and then keep writing code without checks, because come one close never fails. If you do check you are the weird edge case. The one people whisper about "why does he have to be such an annoying pedant". And while I'm ranting, the same goes for undefined behavior. Of course you shouldn't rely on undefined behavior. Of course of course. But maybe, just a little pointer casting is fine?

ncmncm 6y ago
No one checks close()'s result because it lies. What should you write, while (close(fd) != 0) {} ? Or does it risk getting an infinite loop if, e.g., some network connection disappears? Maybe if (fsync(fd) == 0) while (close(fd) != 0) {} else abort(); But maybe fsync is interruptible too. Ultimately there is nothing strictly correct to write, and you should just not really count on files being closed before process termination, but call close() as an optimization not to leak too many fds.
- cesarb 6y ago
  You cannot retry close(). Quoting from https://man7.org/linux/man-pages/man2/close.2.html#NOTES https://man7.org/linux/man-pages/man2/close.2.html#NOTES Retrying the close() after a failure return is the wrong thing to do, since this may cause a reused file descriptor from another thread to be closed. This can occur because the Linux kernel always releases the file descriptor early in the close operation, freeing it for reuse; the steps that may return an error, such as flushing data to the filesystem or device, occur only later in the close operation. That is, once you call close(), the file descriptor is always freed (even if close() returns an error), and can be reused for an open() or similar in another thread. Retrying the close() will fail with EBADF in the best case, and close something another thread has opened in the worst case.
  - OskarS 6y ago
    So what is the point of it having a return value at all then?
- rectang 6y ago
  > No one checks close()'s result because it lies. And since nobody checks the result of close and very few check the result of every single write operation, many "disk full" errors go unnoticed.
  - ncmncm 6y ago
    If you want to know whether your writes are getting out, you will get a much more reliable indication from fsync(). So, instead of exhorting people to check close()'s result, you should exhort them to fsync() first and check that result. A very old programming principle says, "Never check for any failure you are not equipped to act on." It is sometimes used as a reminder to ensure you are always so equipped.
    - rectang 6y ago
      My understanding is fsync() will tell you more than close() only when a flush of the OS cache fails to make it to disk. Is there anything else? The problem with calling fsync() is that you have to wait for it to finish. There are many scenarios where the extra data integrity guarantees you get from calling fsync() aren't important.
MaulingMonkey 6y ago
> Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done. malloc is worth null-checking, if only to assert/panic/breakpoint/fatal early and cleanly. Even on linux with overcommit, it'll return null on memory space exhaustion, which can easily happen even in a 64-bit processes if someone feeds your program maliciously crafted data with an oddball size. A crash dump at the point of allocation failure, when the sizes are still on the call stack / in registers, instead of minutes later - when the pointer is actually used - is also a vastly better debugging experience when fuzzing for such bugs. If malicious data can control offsets into the (null) pointer, an allocation failure can turn into an exploitable buffer overflow (just make the offset large enough), even if the offset is bounds-checked against the "expected" (even larger) allocation size.
- im3w1l 6y ago
  That's a very interesting idea for exploitation, I bet a lot of things are vulnerable to that. But anyway I doubt that people do it even though they should.
  - MaulingMonkey 6y ago
    > But anyway I doubt that people do it even though they should. A lot of codebases certainly skip such checks. But it's also fairly common to at least have a wrapper function that mallocs + triggers a fatal error if it returned null - and then to use that wrapper throughout the codebase. Gamedev tends to roll it's own allocators anyways, or need oddball system allocators for some resources, so they typically have a decent place to stick such checks already.
einpoklum 6y ago
> Everyone nods and say you should, and then keep writing code without checks, Well, I would take points off for not checking return values, although TBH my basic course did not have file I/O (they would use input and output redirection instead, to keep the programs focused on more fundamental things). > And while I'm ranting, the same goes for undefined behavior. Of course you shouldn't rely on undefined behavior. Of course of course. But maybe, just a little pointer casting is fine? That's a trickier subject, because it's not just the hassle of checking return values, it may mean your whole approach to implementing something may go down the drain. Also, it's much less obvious; and compilers don't make an effort to warn you about it; and telling the difference between _undefined_ and _implementation-defined_ behavior is an art which few master (and I'm not even one of those few). This is entertaining to watch regarding UB: CppCon 2017: Piotr Padlewski “Undefined Behaviour is awesome!” https://www.youtube.com/watch?v=ehyHyAIa5so https://www.youtube.com/watch?v=ehyHyAIa5so

by im3w1l 6y ago

ncmncm 6y ago

No one checks close()'s result because it lies. What should you write, while (close(fd) != 0) {} ? Or does it risk getting an infinite loop if, e.g., some network connection disappears? Maybe if (fsync(fd) == 0) while (close(fd) != 0) {} else abort(); But maybe fsync is interruptible too. Ultimately there is nothing strictly correct to write, and you should just not really count on files being closed before process termination, but call close() as an optimization not to leak too many fds.

cesarb 6y ago
You cannot retry close(). Quoting from https://man7.org/linux/man-pages/man2/close.2.html#NOTES https://man7.org/linux/man-pages/man2/close.2.html#NOTES Retrying the close() after a failure return is the wrong thing to do, since this may cause a reused file descriptor from another thread to be closed. This can occur because the Linux kernel always releases the file descriptor early in the close operation, freeing it for reuse; the steps that may return an error, such as flushing data to the filesystem or device, occur only later in the close operation. That is, once you call close(), the file descriptor is always freed (even if close() returns an error), and can be reused for an open() or similar in another thread. Retrying the close() will fail with EBADF in the best case, and close something another thread has opened in the worst case.
- OskarS 6y ago
  So what is the point of it having a return value at all then?
rectang 6y ago
> No one checks close()'s result because it lies. And since nobody checks the result of close and very few check the result of every single write operation, many "disk full" errors go unnoticed.
- ncmncm 6y ago
  If you want to know whether your writes are getting out, you will get a much more reliable indication from fsync(). So, instead of exhorting people to check close()'s result, you should exhort them to fsync() first and check that result. A very old programming principle says, "Never check for any failure you are not equipped to act on." It is sometimes used as a reminder to ensure you are always so equipped.
  - rectang 6y ago
    My understanding is fsync() will tell you more than close() only when a flush of the OS cache fails to make it to disk. Is there anything else? The problem with calling fsync() is that you have to wait for it to finish. There are many scenarios where the extra data integrity guarantees you get from calling fsync() aren't important.

MaulingMonkey 6y ago

> Take for instance malloc. It can return NULL but you don't check for it, because that's just the way it's done. malloc is worth null-checking, if only to assert/panic/breakpoint/fatal early and cleanly. Even on linux with overcommit, it'll return null on memory space exhaustion, which can easily happen even in a 64-bit processes if someone feeds your program maliciously crafted data with an oddball size. A crash dump at the point of allocation failure, when the sizes are still on the call stack / in registers, instead of minutes later - when the pointer is actually used - is also a vastly better debugging experience when fuzzing for such bugs. If malicious data can control offsets into the (null) pointer, an allocation failure can turn into an exploitable buffer overflow (just make the offset large enough), even if the offset is bounds-checked against the "expected" (even larger) allocation size.

im3w1l 6y ago
That's a very interesting idea for exploitation, I bet a lot of things are vulnerable to that. But anyway I doubt that people do it even though they should.
- MaulingMonkey 6y ago
  > But anyway I doubt that people do it even though they should. A lot of codebases certainly skip such checks. But it's also fairly common to at least have a wrapper function that mallocs + triggers a fatal error if it returned null - and then to use that wrapper throughout the codebase. Gamedev tends to roll it's own allocators anyways, or need oddball system allocators for some resources, so they typically have a decent place to stick such checks already.

einpoklum 6y ago

> Everyone nods and say you should, and then keep writing code without checks, Well, I would take points off for not checking return values, although TBH my basic course did not have file I/O (they would use input and output redirection instead, to keep the programs focused on more fundamental things). > And while I'm ranting, the same goes for undefined behavior. Of course you shouldn't rely on undefined behavior. Of course of course. But maybe, just a little pointer casting is fine? That's a trickier subject, because it's not just the hassle of checking return values, it may mean your whole approach to implementing something may go down the drain. Also, it's much less obvious; and compilers don't make an effort to warn you about it; and telling the difference between _undefined_ and _implementation-defined_ behavior is an art which few master (and I'm not even one of those few). This is entertaining to watch regarding UB: CppCon 2017: Piotr Padlewski “Undefined Behaviour is awesome!” https://www.youtube.com/watch?v=ehyHyAIa5so https://www.youtube.com/watch?v=ehyHyAIa5so