Constructing color codes using snprintf for memory safety

[garretcarrot]

Summary: Running the choose binary terminates in a SIGILL (illegal instruction). I'm hoping this change keeps that from happening!

I originally experienced this when looking at Homebrew/homebrew-core#59669. I have some more notes on how I arrived at this point, if anyone is interested. But I'll leave out the boring details, and stick to the relevant points. I'd be happy to answer any questions if I can.

Reproducing the error

These are the steps I took on the default branch. When I do them with applied changes, I see the expected output.

1. Build from source:

   > xcodebuild SDKROOT= SYMROOT=build clean
   > xcodebuild SDKROOT= SYMROOT=build -configuration Release build
   

2. Run the executable. Actual output:

   # Bash
   $ /usr/local/Cellar/choose-gui/1.2/bin/choose -h
   Illegal instruction: 4
   
    # Zsh
    % /usr/local/Cellar/choose-gui/1.2/bin/choose -h
    zsh: illegal hardware instruction  /usr/local/Cellar/choose-gui/1.2/bin/choose -h
   
    # fish
    > /usr/local/Cellar/choose-gui/1.2/bin/choose -h
    fish: '/usr/local/Cellar/choose-gui/1.…' terminated by signal SIGILL (Illegal instruction)
   

3. Expected output:

   > build/Release/choose -h
   usage: build/Release/choose
   --snip--
   

System Software Overview

Versions:

• System: macOS 10.15.6 (19G2021)
• Kernel: Darwin 19.6.0
• Xcode: Version 11.7 (11E801a)

[chipsenkbeil]

Great find! Thanks!

[chipsenkbeil]

See new PR for version 1.2.1 (contains your fix) for homebrew: Homebrew/homebrew-core#61618

[chipsenkbeil]

Also, @garretcarrot, I'm interested in how you figured out the culprit here, in case you want to share one day. 😄

[garretcarrot]

I figured out where the illegal instruction was happening using lldb, but not why it was happening -- until now. Had I known C better, it would have been clear from reading the code: strings are NUL-terminated, so the format string "%2X%2X%2X" is actually seven bytes, not six, and the sprintf was failing to write past the end of the array.

(So there was also a bug in the new code; the fix for that is in #22.)

LLDB actually points out that the process ends up in __sprintf_chk which I did not know about, so at least there's some guard against unsafe memory access!