On Tue, Jul 28, 2020 at 12:52:41PM +0200, Danny Milosavljevic wrote:
> some comments on the lastest patch:

Thank you for reviewing the patch!

> * The entire alsa-lib seems to use the idiom "malloc and then strcpy", or
> "malloc and then sprintf", or, worse, "malloc, strcpy and multiple strcat".
> These are a buffer overflow waiting to happen (when changing part of those
> while doing ongoing maintenance;  also the places where they use "+" is not
> checked for overflow).  That said, if they do it, we can do it that way, too.

This confirms what I felt — it's hard to feel confident about the bounds
checking in this code. It seems to be based on the names of the plugin
libraries not exceeding some magic length. It's hard to balance "doing
the right thing" and using upstream's idioms.

When writing the patch, my investigation into the code made decide that
it would not overflow, but now I don't remember why I thought that.

> * The environment variable GUIX_ALSA_PLUGIN_DIRS is only checked if the
> respective file does not exist in alsa-lib.  That is not how environment
> variables usually work--it should be possible to override built-in things
> by setting this environment variable, too.

Good point. I don't remember now if I specifically decided to do things
this way or if it was a side effect of where I inserted the new code.

> * Instead of alloca and strcpy, can just use strdupa.

I didn't know about this function, thanks.

> * strtok_r man page states that the first argument should be NULL on the
> non-first calls.  You do that already, but maybe add a comment why that
> is done where it's set to NULL.

Right.

> * strtok_r man page states that "On some implementations, *saveptr is required
> to be NULL on  the first call to strtok_r() that is being used to parse str.".
> So I'd use "char* saveptr = NULL;"

My Linux 4.16 man pages from Debian don't contain this note. Good to
know!

> * Instead of malloc and sprintf, could just use asprintf.  But they don't,
> so let's not either, for easier review.  Also, magical value 32... sigh.
> Well, they do it, too.

Right...

> * If GUIX_ALSA_PLUGIN_DIRS contained for example "a:" then it would search
> "a" and "/", right?  OK as long as we want that.

I don't remember how it behaves anymore... I'll look into this and
decide.

Thanks again for the review!