Just truncate the initial promises back to the final ones after pledging
for the first time, saving code and memory.
Assign `ptr' in all initial `seprintf()' calls for consistency while
here.
No need to wait for so long.
This also brings all the pledge code on one screen and helps show how
ircConnect() is the only relevant part in between initial and final
promises.
`-T[format]' is not possible with getopt(3) but getopt_long(3) supports
"T::" exactly for that, so make the command line option go in line with
configuration files and documentation.
While here, check `has_arg' explicitly as getopt_long(3) only documents
mnemonic values not numerical ones.
Otherwise "/exec sh </dev/tty" takes over and catgirl must effectively
be killed to stop the madness; with this diff:
catgirl input| /exec sh </dev/tty
catgirl output| /bin/sh: cannot open /dev/tty: Device not configured
catgirl output| Process exits with status 1
Do the same for `-C/Copy', `-N/notify' and `-O/open' alike.
No point in creating (sub)directories when the given root failed already
as is the case when e.g. XDG_DATA_HOME/catgirl/ itself is bogus
(cleaned stderr intermangled with ncurses setup/catgirl output):
$ env -i TERM=xterm XDG_DATA_HOME=/ ./catgirl -h irc.hackint.eu -n nobody -l
catgirl: //catgirl/: Permission denied
catgirl: //catgirl/log: No such file or directory
catgirl: //catgirl/log/hackint: No such file or directory
catgirl: //catgirl/log/hackint/NickServ: No such file or directory
catgirl: //catgirl/: Permission denied
catgirl: //catgirl/log/hackint/NickServ/2021-06-13.log: No such file or directory
Avoids another small TOCTOU. Rewind before loading since "a+" sets
the file position at the end. Remove unnecessary fseek after
truncation, since "a+" always writes at the end of the file.
All opening happens before unveil/pledge and the file handle is kept
open read/write so it can be used without any pledge.
Simpler/less code and less chances to write other files (accidentially).
Opening the same file *path* twice is a TOCTOU, although not a critical
one: worst case we load from one file and save to another - the impact
depends on how and when catgirl is started the next anyway.
More importantly, keeping the file handle open at runtime allows us to
drop all filesystem related promises for `-s/save' on OpenBSD.
uiLoad() now opens "r+", meaning "Open for reading and writing." up
front so uiSave() can write to it. In the case of a nonexistent save
file, it now opens with "w" meaning "Open for writing. The file is
created if it does not exist.", i.e. the same write/create semantics as
"w" except uiLoad() no longer truncates. existing files.
uiSave() now truncates the save file to avoid appending in general.
After TLS cert/key files, the save file is the only file being read from;
do so before pleding and drop the "rpath" promise all together: log files
will only be created and written to.
Both ssl(8) as well as ncurses(3) related files are now read completely
by the time of ircConfig() and uiInitEarly() respectively, so read
access to the filesystem is no longer needed at all unless the "log" or
"save" options are used.
Previous tls_default_ca_cert_file(3) hoisting makes this possible: all
TLS related files are fully loaded into memory by ircConfig() such that
ircConnect() will not do any file I/O.
Call ircConfig() before pledge(2) in the `-o' "print cert" case so this
works out -- that order should have been preserved in the previous
a989e15 "OpenBSD: hoist -o/printCert code to simplify" but fixing it now
nicely demonstrates the achivement even more so.
tls_connect_socket(3) in ircConnect() does that by default already
unless tls_config_set_ca_file(3) was used.
Loading CA certificates before connecting makes no practical difference
except on OpenBSD where this allows for tighter unveil und pledge setups
now that all required (TLS related) file I/O is finished by the time
ircConnect() gets to do network I/O.
In case of the hidden `-!' insecure flag which is implied by `-o' to
print server certificates and exit, loading root certificates is not
required at all; likewise, using explicit self signed server
certificates will not involve certificate authorities either, hence load
them only if needed.
It is technically undefined behavior (see C11 6.5.6p8) to construct
a pointer more than one past the end of an array. To prevent this,
compare n with the remaining space in the array before adding to
ptr.
Based on seprint(2) from Plan 9. I'm not sure if my return value
exactly matches Plan 9's in the case of truncation. seprint(2) is
described only as returning a pointer to the terminating '\0', but
if it does so even in the case of truncation, it is awkward for the
caller to detect. This implementation returns end in the truncation
case, so that (ptr == end) indicates truncation.
catgirl needs:
- "stdio tty" at all times
- "rpath inet dns" once at startup for terminfo(5) and ssl(8)
- "proc exec" iff -R/restrict options is disabled
- "rpath wpath cpath" iff -s/save or -l/log options is enabled
Status quo: catgirl starts with the superset of all possible promises
"stdio rpath wpath cpath inet dns tty proc exec", drops offline with
"stdio rpath wpath cpath tty proc exec" and possibly drops to either of
"stdio rpath wpath cpath tty", "stdio tty proc exec" or "stdio tty"
depending on the options used.
Such step-by-step reduction is straight forward and easy to model along
the process runtime, but it comes with the drawback of starting with
too broad promises right from the beginning, i.e. `catgirl -R -h host'
is able to execute code and write to filesystems even though it must
never do so according the (un)used options.
Lay out required promises up front and pledge in two stages:
1. initial setup, i.e. fixed "stdio tty" plus temporary "rpath inet dns"
plus potential "rpath wpath cpath" plus potential "proc exec"
2. final rutime, i.e. fixed "stdio tty"
plus potential "rpath wpath cpath" plus potential "proc exec"
This way the above mentioned usage example can never execute or write
files, hence less potential for bugs and more accurate modelling of
catgirl's runtime -- dropping "inet dns" alone in between also becomes
obsolete with this approach.
initscr(3) in uiInitEarly() attempts more than /usr/share/terminfo/, see
`mandoc -O tag=TERMINFO ncurses`.
Even though non-default terminfo handling seems rare and it is unlikely
to have ever caused a problem for catgirl users on OpenBSD, the current
is still wrong by oversimplifying it.
Avoid the entire curses/unveil clash by setting up the screen before
unveiling.
Nothing but the TLS handshake is required, so skip all other setup.
On OpenBSD, unveil() handling needs fixing which will involve code
reshuffling -- this is the first related but standalone step.
Also pledge this one-off code path individually such with simpler and
tighter promises while here.
The (not perfectly obvious) way catgirl crafts directories gets triggered
by unveilAll() even if no passed option requires filesystem access:
$ env -i TERM=xterm ./catgirl -h irc.hackint.eu -R -n nobody
catgirl: HOME unset
Here unveil(2) is used due to the "restrict" option, but besides terminfo(5)
and certificates catgirl does not need any other files, yet it tries to init
the data path -- passing XDG_DATA_HOME=/var/empty makes above invocation work
showing how the then successful path setup is not required.
Fix this by not unveiling the unneeded data path in the first place.
"username" alone is ambiguous and without jumping to ENVIRONMENT
explaining the use of USER, catgirl's user- and nickname options read
like pointing at each other:
-n nick | nick = nick
Set nickname to nick. The default nickname is the user's name.
[...]
-u user | user = user
Set username to user. The default username is the same as the
nickname.
Clarify that `-n' does *not* default to `-u's value.
When waddnstr is called with a string that would extend past the
end of the window, the string is truncated, the cursor remains at
the last column, and ERR is returned. If this error is ignored and
the loop continues, the next call to waddnstr overwrites the character
at this column, resulting in a slight visual artifact. When the
window is too small to fit the full status line, it is effectively
truncated by one space on the right, since the string shown for
each channel begins with a space. Additionally, if the last window
is the current window, the space is shown with a colored background.
To fix this, when waddnstr returns ERR, exit the loop in styleAdd()
early return -1 to propogate this error down to the caller.
`-H 0,0`/"hash = 0,0" makes catgirl mostly colorless which is great,
but topic changes still hardcode brown/green colors to show differences
which is usually not desired by users (like me) disabling colors.
Go for a less eye stressing topic change message that shows both old
and new in reverse video with default terminal colors.
This isn't perfect, other parts of catgirl still hardcode colors and
`-H 0,0`/"hash = 0,0" was never meant to disable colors completely, but
topics change often enough that avoiding less readable^Waccessible topic
diffs seems sensible enough.
NB: parseHash() is brittle and "0,0" is not the only value disabling
colors...
Using the +draft/reply client tag, which is supported by BitBot.
This hides the bot's replies to ignored users or ignored bot command
messages.
This commit is dedicated to the land of Estonia.
I avoided defaulting MANDIR to /usr/local/man because I thought it
didn't work on GNU/Linux and users would be confused, but it turns
out man-db's default configuration includes both /usr/local/man and
/usr/man, so ${PREFIX}/man is a sensical default.
Klemens Nanni
C. McEnroe
Michael Forney