ArgumentParser spuriously exits with a zero status on certain errors

[lorentey]

This package uses the errorCode property of CustomNSError errors as the process exit status.

      // MessageInfo.swift:109
      case let error as CustomNSError:
         self = .other(message: error.localizedDescription, exitCode: Int32(error.errorCode))

The purpose of errorCode is merely to differentiate error cases within the same error domain; it is not appropriate to use it as an exit status. The process exit status has a limited range (either 8 or 32 bits depending on the syscall used to retrieve it); additional bits get silently truncated. As a result, when the classic wait/waitpid syscalls are used, ArgumentParser processes may appear to exit with a zero status (EXIT_SUCCESS) even if they failed.

Additionally, an errorCode that doesn't fit in an Int32 leads to a fatal runtime error.

The broken behavior was introduced in #244. One easy way to fix this is to revert that change.

One way to correctly implement #230 is to define a custom mix-in error protocol with an explicit property for exit codes that is kept distinct from errorCode.

ArgumentParser version: 0.3.2
Swift version: Latest stable

Checklist

• If possible, I've reproduced the issue using the main branch of this package
• I've searched for existing GitHub issues

Steps to Reproduce

import Foundation
import ArgumentParser

enum MyError: Int, CustomNSError {
  case foo = 256
  case bar = 10_000_000_000

  static var errorDomain: String { "MyError" }
  var errorCode: Int { self.rawValue }
  var errorUserInfo: [String : Any] { [:] }
}

struct Command: ParsableCommand {
  @Flag
  var trap: Bool = false

  func run() throws {
    throw trap ? MyError.bar : MyError.foo
  }
}

Command.main()

Expected behavior

I expect ArgumentParser.main to exit normally with a non-zero status whenever run throws an error it doesn't otherwise handle.

On the bash/fish/zsh prompt, I expect to see:

$ swift run test
$ echo $?
1   # Or some other non-zero value

$ swift run test --trap
$ echo $?
1   # Or some other non-zero value

Actual behavior

Depending on the errorCode value, the subprocess appears to exit with a zero status, or exits with a signal instead.

With the bash/zsh installations on default macOS, and fish installed from homebrew, I get:

$ swift run test
$ echo $?
0
$ swift run test --trap
Swift/Integers.swift:3550: Fatal error: Not enough bits to represent the passed value
Illegal instruction: 4

Exiting with (what appears to be) a zero exit code when the process did not succeed is a very serious problem.

Shells (and other processes) that use the newer waitid syscall can get access to the full 32 bits of the exit code, so they'd report a nonzero code here. However, all shells I tried so far are still using the older wait/waitpid syscalls; we ought to be very conservative about what values we pass to exit.

[SergeyPetrachkov]

From the "fundamental" point of view I cannot agree more that the issue exists and somewhat possible. But from the point of view where swift-argument-parser is a utility library for developers to create console tools - it's highly unlikely that the given example will ever take place when someone will ever create a custom error with more codes than Int32 capacity.

[finestructure]

That's interesting and certainly good to know! One mitigating factor is that, at least for my use case, I set the error codes myself. So knowing this I can at least step around the issue by staying within 8 bits (which incidentally I had). Thanks for raising this.

@SergeyPetrachkov - my understanding is it's not just Int32 but also 8 bit truncation - see the first error - 256 - returning success instead of failure. That's quite bad.

[SergeyPetrachkov]

@finestructure yeah, I see...

[finestructure]

Maybe in the interim (until there's a better way to do this), errorCodes exceeding 256 could trap/throw here (in MessageInfo.swift:107):

        // check errorCode and throw sth else or fatalError if >= 256?
        self = .other(message: error.localizedDescription, exitCode: Int32(error.errorCode))

[natecook1000]

@lorentey Thanks for this! It looks like there are a few separate actions here:

1. CustomNSError.errorCode should not be used as the exit code. I’ll revert Support Exit codes from thrown CustomNSError conformers #244.
2. We should add a CustomExitStatus (or some better name) protocol so that we’re still providing the functionality identified in Return codes for custom error types #230.
3. We should better handle unusual, out of scope, error codes.

Re: out of scope error codes, I think our options are to trap or to clamp to some other value. I’d prefer not to trap, and in at least some contexts, it looks like 255 is reserved for out of range exit statuses. That might be an appropriate solution here — to just take any error code outside of the range 0..<256 and map it to 255.

[lorentey]

Having thought about this a little more, I think a new protocol may not be the best approach:
#230 (comment)