Merge topic branch t/ls-l into pu

Started on 2024-03-17

A single commit which deprecates the chunk table and mbox listing
modes. The chunk table can still be listed with para_afh(1), and the
mbox format was never really useful to begin with. The idea is to
remove support after 0.8.0 has been released.

<!--
- The chunk table and mbox listing modes of the ls server command
have been deprecated. They still work, but a warning is issued to
inform the user about the deprecated option.
-->

* refs/heads/t/ls-l:
  server: Deprecate -l=c and -l=m of com_ls().

Merge topic branch t/home into pu

Started on 2024-03-20

* refs/heads/t/home:
  f
  Replace get_homedir() by get_confdir().
  Consult $HOME rather than calling getpwuid(),

Merge topic branch t/for-maint into pu

This branch accumulates small but significant hotfixes for known bugs
in the most stable integration branch, "maint". Only serious issues
are fixed here, and no development takes place on this branch.

The purpose of the branch is to give such urgent fixes at least
a small amount of testing in pu before they are incorporated into
maint. During this time the patch can be tweaked, and the documentation
can be improved. Usually the commits of this branch are merged quickly,
however, and it it perfectly normal if this branch is empty.

* refs/heads/t/for-maint:
  play: Fix integer overflow in com_ff().

Merge topic branch t/btr into next

Some helpers such as btr_get_input_queue_size() are very expensive
if the buffer tree contains many buffers, which is the case for mp3
and aac streams. This is noticeable in particular with para_play,
which spends more time in the btr subsystem than in the mp3 decoder
if the file is large enough. This branch contains a few patches to
improve that.

* refs/heads/t/btr:
  btr: Streamline the documentation of buffer_tree.h.
  Constify buffer tree API.
  btr: Merge buffers on insertion.
  btr: Speed up btr_node_status().
  Speed up mp3dec filter.

Merge branch 'master' into next

* master:
  client: Fix memory leak on exit.
  ipc: Remove pointless initialization in shm_get_shmmax().

btr: Streamline the documentation of buffer_tree.h.

Nothing major, just a few minor English issues.

Constify buffer tree API.

A lot of functions of the buffer tree API don't modify the memory
referenced by the pointers passed. This patch marks these pointer
arguments as constant.

btr: Merge buffers on insertion.

Currently add_btrb_to_children() simply adds the given buffer
reference to the input queue of all children of the given node even
if the newly added buffer reference points memory adjacent to the
previously added buffer.

Since several helpers iterate over all buffer references, performance
suffers, given enough buffer references. So merge buffers when
possible.

User time: 147s -> 83s, speedup: 44%.

btr: Speed up btr_node_status().

Currently we sum up the sizes of all buffers in the input queue just to
determine if the total size exceeds a small threshold. That's silly
and expensive if there are many buffers. Fix that by introducing
a helper which breaks out of the loop as soon as know the answer
because the threshold is exceeded.

User time: 150s -> 147s, speedup: 2%

Speed up mp3dec filter.

Avoid calling btr_get_input_queue_size() as this is potentially
expensive (depending on the number of chunks in the input queue)
and unnecessary.

The user time of para_play for a ~4m long mp3 file on a very slow
machine went down from 168s to 150s. Speedup: 11%

client: Fix memory leak on exit.

This triggers only if para_client is run interactively. The leak is
harmless, but still..

ipc: Remove pointless initialization in shm_get_shmmax().

The subsequent read() call overwrites the buffer.

play: Fix integer overflow in com_ff().

The product of the number of chunks and the number of seconds to skip
may overflow the 32 bit multiplication. Add a cast to force 64 bit
arithmetics to avoid the overflow.

f

Replace get_homedir() by get_confdir().

Every single caller of get_homedir() uses the returned string to
create a path below ~/.paraslash, so we might as well return this path,
simplifying the callers.

Consult $HOME rather than calling getpwuid(),

Quiting getpwnam(3):

An application that wants to determine its user’s home
directory should inspect the value of HOME (rather than the value
getpwuid(getuid())->pw_dir) since this allows the user to modify
their notion of "the home directory" during a login session.

Change the function to return const char *, i.e. a pointer to the
environment rather than a copy that must be freed by the caller.
Document that the caller must not modify or free this string and fix
all callers accordingly.

build: Rename all_objs -> dep_objs.

The old name is misleading since there are additional objects which
correspond to the C files generated by lopsub. Those use different
dependency tracking.

Simplify the code a bit by creating the list in one step.

server: Deprecate -l=c and -l=m of com_ls().

These are not very useful. The implementation will be removed after
0.8.0.

build: No longer check for CRYPTO_cleanup_all_ex_data().

Recent openssl versions no longer declare this unless the openssl API
compatibility version is set explicitly, in which case it gets defined
as a macro which does nothing. Either way, the check in configure.ac
and the call in openssl.c can go away.

build: Construct list of executables in Makefile.

This is actually easier and removes some duplication as we currently
have two copies of the logic which decides whether or not to build
each executable. We lose the summary output at the end of the configure
output, but that was incomplete anyway.

build: Construct audioc object list in Makefile.

This removes the last caller of add_dot_o() in configure.ac, so remove
this function.

build: Construct write object list in Makefile.

build: Construct play object list in Makefile.

build: Construct afh object list in Makefile.

build: Construct filter object list in Makefile.

build: Construct recv object list in Makefile.

para_recv is always built, which makes the patch comparably small.

build: Construct gui object list in Makefile.

build: Construct mixer object list in Makefile.

build: Construct audiod and client object list in Makefile.

Both executables are built iff CRYPTOLIB is set, so they can be
handled in one conditional block of Makefile.real.

build: Construct server and upgrade_db object list in Makefile.

Currently configure.ac defines variables for the list of objects of
each executable and exports these via AC_SUBST to Makefile.in. The
lists are then extended and munged further in Makefile.real.

It it easier to construct the lists only in the makefile. So, as
a first step, stop to export the object list of para_server and
para_upgrade_db. Instead, export the variables needed to construct
the list in the makefile.

build: Simplify condition for building para_server.

We fail the configure script earlier if flex or bison are not
installed, so we don't need to repeat this check here.

build: Simplify ogg helpers of configure.ac.

There is no need to define NEED_OGG_OBJECTS and friends as a function
because a simple variable will do as well.

build: Remove AUDIOD_AUDIO_FORMATS_ENUM.

No code actually refers to the AUDIO_FORMAT_$FOO values of this
enumeration. Its only purpose is to define the number of supported
audio formats (as the last constant of the enum). We may as well
employ ARRAY_SIZE() for that, so kill the enum and remove the loop
from configure.ac which generates it.

The related audio_formats array, which contains the names of the
supported audio formats, is only used in audiod.c, so we can remove
the declaration from audiod.h and make it static.

build: Remove _errlist in shell variables of configure.ac.

This used to make sense when we had two types of variables per
executable, but those times are long gone.

build: yy_src_dir and yy_build_dir can be simply expanded.

Simply expanded variables should be preferred whenever possible since
they are more intuitive and a bit faster.

build: Remove info about ucred from configure output.

The $have_ucred variable was removed six years ago, but the stale
summary line remained. Remove it now.

Fixes: 738d19a4cfb194479e9e0c4a474df6b62c57f4b4

paraslash 0.7.3

Update copyright year.

gui: Fix off-by-one in colorful blackness theme.

Without this, on a 80 character window and the three-digit amp value,
the value gets truncated unnecessarily.

audiod: Improve stat_client_add().

Let it receive a bool rather than an int and dedox its documentation
because it's a static function.

string.c: Don't fall back to /tmp in para_homedir().

This can only lead to trouble. If we can't get the path to the home
directory, something is deeply wrong and we really should abort.

create_argv_offset(): Use arr_zalloc().

There is no point in initializing the allocated array manually,
and it does not hurt to initialize also the last element.

Compile with -Wsuggest-attribute=malloc.

We already employ this attribute extensively to help the compiler
improve optimization. However, a few malloc-like functions were not
yet marked with __malloc. Fix that and enable the warning to make
sure that new malloc-like functions get marked.

Since not all supported compilers know about this warning option, we
need to check at compile time whether the option is supported. Thanks
to the existing cc-option make(1) function, this is a simple one-liner
for Makefile.real.

server: No longer accept "sideband" and "aes_ctr128" features.

Both features are used unconditionally since commit d44413588dd7
(v0.6.3-27) from three years ago when the client stopped to request
the feature. We don't need to support clients older than that any more,
so fail the request if these features are still requested.

Clarify the comment about the sha256 feature while at it.

gui: Clear status bar after pressing <End>.

Without this a stale message such as

"scrolled view: 1-36/42"

remains although the window has been updated to show the bottom of
the ringbuffer.

Merge topic branch t/crypt-cleanups into master

This bunch of mostry trivial changes can be merged early, before the
openssl code is converted to use the EVP API. The topic was cooking
in next for six months.

* refs/heads/t/crypt-cleanups:
  openssl: Assign bignums in canonical order.
  openssl: Unify naming of public key structures.
  openssl: Rename read_private_rsa_params() -> read_openssh_private_key().
  openssl: Rename read_rsa_bignums() -> read_public_key().
  openssl: Dedox crypt_init().
  server: Improve "loading pubkey" log message.
  gcrypt: Remove pointless state variable.
  client: Reduce line length.

Merge topic branch t/ls-L into master

A single patch which implements the new --limit option for the ls
server command. The confict in aft.c was trivial to fix.

* refs/heads/t/ls-L:
  server: Implement ls --limit.

Merge topic branch t/afs-select into master

A single patch which silences the select command. The merge conflicted
in afs.c, but that was trivial to resolve.

* refs/heads/t/afs-select:
  server: Implement select -verbose.

Merge topic branch t/afs-ls-a into master

A new feature for the ls command. Unfortunately, several bugs were
found after the topic graduated to next, so the series contains a few
fixup commits on top of the single patch which implements the feature.

* refs/heads/t/afs-ls-a:
  afs: Really fix memory leak in mood_load().
  afs: Fix memory leak in mood_load().
  playlist: Fix error handling of playlist_load().
  server: Fix NULL pointer dereference in com_ls().
  Implement ls --admissible=m/foo.

Merge topic branch t/fd into master

A rash of patches which clean up a good part of fd.c. Nothing major
here, mostly simplifications and documentation improvements.

* refs/heads/t/fd:
  fd: Simplify and move for_each_file_in_dir().
  fd.c: Improve error checking of para_mkdir().
  fd: Revamp para_mkdir().
  fd: Improve read_pattern(), rename it to read_and_compare().
  fd: Remove log message from para_munmap().
  fd: Open-code para_chdir().
  fd: Remove file_exists().
  fd: Improve documentation of xwritev().
  fd: Improve documentation of write_all().
  fd: Improve documentation of write_va_buffer().

server: Fix memory leak at exit.

If a playlist is open on exit, we miss to free it. Not a real leak,
but still..

afs: Really fix memory leak in mood_load().

The previous fix was insufficient as we also have to destroy the
score table in the success case, so move the freeing to destroy_mood().

Fixes: bb0aec0963b1b2da617aebda26deca576684436c

Merge branch 'maint'

A single patch which adds a missing error check.

* maint:
  audiod: Fix error checking in init_default_filters().

afs: Fix memory leak in mood_load().

In the result != NULL case we open a fresh score table but miss to
close it if add_to_score_table() fails. This should never happen,
but still..

Fixes: 2d2637cb4c9ab76fea6bc336b9af88fd00bf5e08

playlist: Fix error handling of playlist_load().

We open a fresh score table if the result pointer is not NULL,
indicating that we are called from com_ls() (with -a=p/foo) rather
than from com_select(). However, if an error occurs afterwards, we
call score_close() unconditionally. This is wrong in the result ==
NULL case (com_select()) because it closes the global score table
which is expected to stay open.

The result is a UAF, which is diagnosed by valgrind as follows:

==4767== Invalid read of size 4
==4767==    at 0x408C51E: osl_add_and_get_row (osl.c:1216)
==4767==    by 0x408CA99: osl_add_row (osl.c:1348)
==4767==    by 0x8060648: score_add (score.c:116)
==4767==    by 0x805F08C: add_to_score_table (mood.c:451)
==4767==    by 0x805FA3E: mood_load (mood.c:650)
==4767==    by 0x8057ECF: activate_mood_or_playlist (afs.c:447)
==4767==    by 0x8059637: com_select_callback (afs.c:1005)

Fixes: 2d2637cb4c9ab76fea6bc336b9af88fd00bf5e08

server: Fix NULL pointer dereference in com_ls().

The previous commit which extended the -a option of the ls command
to accept an optional argument introduced the following flaw: If the
argument of -a corresponds to the name of a mood for which no files
are admissible, the server crashes due to a NULL pointer dereference
because mood_load() leaves the mood instance pointer uninitialized
although it returns zero, indicating success.

This behaviour of mood_load() contradicts the promises made in
its documentation. Fix mood_load() by not special-casing the "zero
admissible files" case, which even simplifies the code a bit. If all
goes well but no files turn out to be admissible, we now open the
score table anyway and set the mood pointer to the allocated mood as
usual. Since get_statistics() may now be called with zero admissible
files, we have to add a check there before dividing by the number of
admissible files,

Fixes: 2d2637cb4c9ab76fea6bc336b9af88fd00bf5e08

audiod_command: Remove dump_stat_client_list().

It is not very useful and can spam the log.

test-lib: Write error output to stderr.

This way one can run

make test > /dev/null

to suppress normal output but still see test failures, if any.

server: Add missing newline in select -v error output.

The error message that lacks the newline can be triggered by runing
"select -v X".

audiod: Fix error checking in init_default_filters().

We ignore the return value of add_filter() instead of assigning it to
ret as usual, then test ret anyway. That's clearly bogus, even more
so since with the old code ret can never be negative at this point, so
the subsequent condition for the subsequent jump to out: is never true.

This bug was introduced 13 years ago.

Fixes: 081ea8699e6a309c44446884b8c3752529d96a25

server: Implement ls --limit.

The new option is quite easy to implement. The patch actually removes
more lines from aft.c than it adds, but this is just because the code
which loops over all matching files was duplicated across the two
branches of the clause which checks whether --reverse was given. The
branches can easily be combined.

server: Implement select -verbose.

Just set ->pbout to NULL if the new option is not given. This supresses
normal output while error messages still make it to the client because
those are sent with afs_error().

openssl: Assign bignums in canonical order.

The order of the bignums stored in the private key is n, e, d, iqmp, p,
q. We read the bignums in this order, so assign the members of the RSA
structure in the same order. This does not really matter, but still..

Deprecate openssl-1.0.

These days everybody should have openssl-1.1 (released in 2016)
or later. So warn if configure detects openssl-1.0 or earlier.

openssl: Unify naming of public key structures.

The pointers to private key structures are all called "priv", but for
their public counterparts we often use "key" rather than "pub". Fix
that to improve readability.

openssl: Rename read_private_rsa_params() -> read_openssh_private_key().

The new name encodes the format of the key in the function name just
like the counterpart read_pem_private_key() does.

openssl: Rename read_rsa_bignums() -> read_public_key().

This makes clear that the function is only used for public encryption.

openssl: Dedox crypt_init().

The API of this function is documented in crypt.h while the comment
in openssl.c describes openssl specific implementation details which
do not need doxygen annotations.

server: Improve "loading pubkey" log message.

Include the path to the key file in the output and make the message
more visible by increasing its severity level.

gcrypt: Remove pointless state variable.

The variable is set but then only used for an unimportant log message.
So let's drop the message and the variable.

client: Reduce line length.

Trivial patch, nothing to see here.

crypt: Deprecate PEM keys.

Users have been advised since 2018 to create RFC4716 keys (see
commit 9e2b093bed5d), and this has been the default key format for
ssh-keygen(1) for a long time as well. Upgrade the severity of the
existing log message from info to warning to encourage users at
runtime to replace their old keys.

client: Fix has_feature().

The function returns bool, so a return value of zero is regarded as
false. Consequently, server feature #0 (sha256) is not detected even
if it is announced by the server, and therefore the client does not
request it. Hence both sides fall back to sha1.

This bug was introduced in paraslash-0.7.2.

Fixes: a0707daa23f8706326aa837f969c20d3e4ee02aa

afs: Fix memory leak in mood_load().

If mood_load() manages to load the mood but does not find any
admissible files, it does not deallocate the mood instance and does
not set up the global current_mood variable either. Plug the resulting
memory leak by destroying the mood also if there are no admissible
files (ret == 0).

Implement ls --admissible=m/foo.

Currently there can be only one score table at a time because the
functions of score.c refer to the global score_table variable.
To implement the new feature, we need to overcome this restriction
so that the callback of the ls command can populate an independent
score table to print its output without interfering with the score
table that is currently active.

This commit changes most functions of score.c to receive an additional
table pointer argument. All current users of the score table pass a
NULL pointer to instruct the functions to operate on the global score
table as before.

However, if the ls command is invoked with an optional mood argument
to -a. the callback calls mood_load(), followed by mood_loop() and
mood_unload(). The former returns an opaque handle which is then
passed to the other two functions to instruct them to operate on the
temporary score table instead of the global one.

To make the feature work for playlists as well, analogous functionality
is implemented in playlist.c. The new mop_loop() of aft.c performs
the disambiguation in a similar way as the activate_mood_or_playlist()
does. It is a bit simpler though, since the ls command does not have
to deal with NULL arguments and does not need to fall back to the
dummy mood.

Merge branch 'maint'

Two fixes for gcc-12 warnings, and another fix for a benign but
embarrassing braino in gcrypt.c.

* maint:
  gcrypt: Fix return value of apc_get_pubkey().
  error.h: Be more careful with error code masking.
  mp3_afh: Drop unused fields from struct mp3header.

gcrypt: Fix return value of apc_get_pubkey().

The function is supposed to return the key size in bytes, but it
returns the number of *bits*. A consequence of this bug is that
RSA keys which are too short to encrypt our 128 byte buffer are not
rejected as they should be. This is not too serious because we'll fail
later during the encryption step. Fix the bug anyway and clarify the
documentation of apc_get_pubkey().

afs: Improve error message in init_admissible_files().

arg may well be NULL here, so print "dummy" in this case.

test-lib: Fix typo in comment.

error.h: Be more careful with error code masking.

It should never happen that two or more of the three special high bits
(osl, lopsub, system) are set in an integer that stores a paraslash
error value, but gcc-12 can't prove this and complains as follows:

error.h:304:28: warning: array subscript 268435456 is above array bounds of 'const char * const[220]' [-Warray-bounds]
  304 |         return para_errlist[num];
      |                ~~~~~~~~~~~~^~~~~

Avoid this warning by always clearing all three special bits.

mp3_afh: Drop unused fields from struct mp3header.

These are never initialized, but still checked in compare_headers(), so
gcc-12 is right when it complains about uninitialized use. Fix this by
simply removing the uninitialized fields and the comparisons. Fix also
a whitespace issue in the definition of compare_headers() while at it.

fd: Simplify and move for_each_file_in_dir().

With only one user it can be static in aft.c. Modify the function
so that it no longer changes the current working directory, remove
para_opendir() because it is unused now, dedox the documentation and
streamline it a bit.

fd.c: Improve error checking of para_mkdir().

The old code returned success in case the pathname existed but was
no directory, so try to improve on this a bit. However, don't be
over-zealous as any pathname based approach won't be bullet-proof
because the file identified by the pathname may change at any time.

fd: Revamp para_mkdir().

It has two callers which both pass the mode value 0777 and contain
extra code to regard the EEXIST error case as a success. Move the
common bits into the wrapper and improve the documentation.

fd: Improve read_pattern(), rename it to read_and_compare().

The old name was a poor choice because the pattern argument actually
is neither a regular expression nor a filename pattern.

More importantly, the function receives a buffer size and tries
to read this many bytes but then compares only the first part of
the received buffer to the expected string. This is a rather weird
calling convention.

The only two callers are the http sender and receiver which both
call the function during the initial handshake where no other data is
available. Thus we can change the function to read only the minimal
amount of data (length of the expected string), and drop the bufsize
parameter.

Remove the unnecessary log message in the error case and streamline
the documentation while at it.

fd: Remove log message from para_munmap().

Low-level functions like this should leave it to the caller to log
the error. Extend the documentation a bit while at it to document
the fact that passing NULL is OK.

fd: Open-code para_chdir().

Another public trivial wrapper that can go away because it has only
a single caller. POSIX says

Upon successful completion, 0 shall be returned. Otherwise, −1 shall
be returned, the current working directory shall remain unchanged,
and errno shall be set to indicate the error.

So the new check against zero is equivalent to the old code which
checked whether the return value is non-negative.

fd: Remove file_exists().

Open-coding this function actually improves code readability. The
function name was a misnomer anyway because any error from the stat()
call (such as EACCES) was reported as "file does not exist".

fd: Improve documentation of xwritev().

This is another essential helper which was poorly documented. In
particular the EINTR and short write cases were not covered, and it
was left open what a return value of zero means.

Omit the DCCP-specific part and the sentence which explains what
EAGAIN means.

fd: Improve documentation of write_all().

The name of this public function is a bit of a misnomer, so be at
least clear in the one-line description what the function does in
the EAGAIN case or if a short write occurs.

fd: Improve documentation of write_va_buffer().

This public function was a bit underdocumented, so explain in more
detail what the function does, and how it differs from fprintf(3).

Consolidate EOF error codes.

Currently we have ~15 error codes which indicate an EOF condition. One
should suffice, so drop all codes except the generic E_EOF and use
that everywhere.

sched: Improve error diagnostics.

Currently the error code of the negative return value
from ->post_monitor() is not logged anywhere under normal
conditions. Instead we log "[dead]" in unlink_and_free_task(). Replace
this by a more meaningful message in task_reap().

server: Improve error diagnostics of com_select().

The attempt to select a non-existing mood or playlist currently
results in

remote: key not found in rbtree

This is a bit cryptic, so be a bit more user-friendy and provide a
paraslash error code for this case.

gui: Improve loglevel message.

When changing the log level, show the new severity string rather than
its numerical representation.

build: Activate warnings and LTO on *BSD.

Newer versions of BSD ship compilers/packages which no longer
throw warnings, so activate the warnings on all platforms. Also LTO
(link-time optimization) seems to work on the test machines now,
so activate this as well.

Tested on FreeBSD-13.1 with clang-13.0.0 and on NetBSD-9.3 with
gcc-7.5.0.

Add two new tests for para_server.

In a (non-public) development branch the add subcommand was broken
although t0004-server exercises this subcommand and reported no
error. The bug went unnoticed because it would only bite when the
subcommand was given a directory to add, but the test specifies a
pathname which refers to a regular file.

To detect such breakage early, add a test which tries to add a
directory and another which exercises the rm command.

paraslash 0.7.2

Manually tweak Doxyfile to squash warnings.

The Doxyfile introduced by the previous commit (created by running doxygen -u)
causes doxygen to complain:

warning: Tag 'TCL_SUBST' at line 260 of file '-' has become obsolete.
 To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: argument 'a4wide' for option PAPER_TYPE is not a valid enum value
Using the default: a4!

This patch should get rid of the warnings.

Update DOXYFILE.

The patched file was created with doxygen -u with no manual editing. We
picked doxygen-1.8.17 although that's already a bit old because this
version ships with Ubuntu-20.04. However, this version generates a file
list (shown on the Documentation page) which lacks the description
column, so we use a self-compiled doxygen executable for the time
being.