Re: CVS commit: src/lib/librumpuser

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Tue, 24 Mar 2020 05:40:13 +0100
    From:        Kamil Rytarowski <n54%gmx.com@localhost>
    Message-ID:  <ea741472-ab1a-9f95-f7ce-72a67608f0c6%gmx.com@localhost>


  | This patch was sitting in the tree since August 2019.

In your tree I assume you mean, it certainly hasn't been in mine.

Was a PR filed about the issue back then?   If so, shouldn't it have
been referenced in the commit message?    If not, one should have been.

If that had (either initially, or in a later add on message) included
the suggested patch, I (and probably several others) would have told you
at the time it was not the correct thing to do - it took all of 30 seconds
of looking at the code to know that it is impossible to be the right fix.

  | This patch was tested to be operational.

I am not surprised at that at all - like I said (but I have still not
analysed things fully) I suspect that all it is doing is writing one \0
on top of a \0 that is already there, and that the correct patch would
have been to simply delete the offending line (and the comments attached
to it).

But that's still a guess.

  | I am aware that writing out of allocated buffer is for some people
  | considered as 'no bug'.

That's not what I said.   What I said was that while it is a bug, and
should be fixed, it was evidently harmless most of the time, as everything
has been working with this bug in it.

There are several possible problems that might exist here, perhaps (maybe
only in some extreme case) the buffer allocated is one byte too short, and
the correct fix would be to add a "+1" to some malloc() call elsewhere in
the code (I doubt that one, but it is possible).

Or perhaps the code that calculates commlen is incorrect, and it should
have had a "-1" included in the expression.

Or perhaps some code that is copying in the args is stopping just before
the \0 that terminates eact string, and failing to copy that one where it
should be, and this extra \0 appended is essential (in which case a correct
fix might be to copy the data into a slightly bigger buffer where there is
space for a \0 to be appended ... or change the arg copying code to include
the \0 rather than exclude it).   I doubt this one too, as I doubt that
things would have worked if this was the case - and your "it works with
the patch" makes it even less likely that this is the problem.

Or perhaps something else.

The point is that this needs to be analysed correctly, not just hacked at
to fix the problem, and that the line of code as it is now is either entirely
reduncant (my guess) or (perhaps only sometimes - and perhaps never in our
current tests) breaks things.

  | I'm fine to see it fixed in a better way than manually injected \0.

OK, then please revert that change (which cannot be the right fix, regardless
of what the right one really is) and file a PR, so it can be fixed properly.

kre

ps: while looking at this I spotted a (complely unrelated) bug in the same
file that should be fixed (no, sanitisers will never find this one, but
human reading easily might) - which I will fix, but I'd prefer to do that
after this fix is reverted, just so those two commit messages are adjacent.