Re: Possible memory corruption (src/timezone/zic.c b/src/timezone/zic.c)

Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> writes:
> Per Coverity.
> CID 1412632 (#1 of 1): Out-of-bounds access (OVERRUN)1.
> overrun-buffer-val: Overrunning buffer pointed to by &c of 1 bytes by
> passing it to a function which accesses it at byte offset 4.

> For some people, Coverity opinions count zero.

This particular complaint seems to match a pattern that Coverity has
been generating a lot lately. I've yet to see one that wasn't a
false positive, so it looks like a Coverity bug to me.

> It doesn't matter if WideCharToMultiByte, it will fail or not, the danger
> exists.
> If WideCharToMultiByte returns 4, memmove will possibly destroy 4 bytes.

This analysis seems to me to be nonsense.

(1) sizeof(char) is one, per the C standard. Therefore, the existing
coding in itssymlink accurately describes the size of the buffer it's
providing. The alternative you propose also accurately describes
the size of the buffer it's providing. It's nonsense to suppose that
one is safer than the other --- if readlink is willing to write past
the specified buffer size, they're both equally dangerous. So this
fix fixes nothing.

(2) As an independent matter, we should worry about whether our
pgreadlink() implementation is capable of writing past the specified
buffer size. I don't think that WideCharToMultiByte will do so;
Microsoft's documentation clearly says that "cbMultiByte" is the
size *in bytes* of the buffer indicated by "lpMultiByteStr".
However it's fair to question whether that bit of code for deleting
"\??\" is safe. I think it is though. Per the Microsoft docs,
the return value of WideCharToMultiByte is:

If successful, returns the number of bytes written to the buffer pointed
to by lpMultiByteStr. If the function succeeds and cbMultiByte is 0, the
return value is the required size, in bytes, for the buffer indicated by
lpMultiByteStr. [ but we aren't passing zero for cbMultiByte ]

The function returns 0 if it does not succeed.
[ and one of the failure cases is: ]
ERROR_INSUFFICIENT_BUFFER. A supplied buffer size was not large enough,
or it was incorrectly set to NULL.

So I don't believe that it will return r > 4 when the supplied buffer size
is only 1. What's going to happen instead is a failure return, because
the string doesn't fit.

Hence, we do have a problem here, which is that pgreadlink is pretty
much always going to fail when used in the way zic.c is using it, and
thus zic is going to fail to recognize symlinks when run on Windows.

The IANA crew are unlikely to care: they're going to say that they're
using readlink() per the POSIX specification for it, and they'll be
right.

So the question for us is whether it's worth trying to make pgreadlink
conform to the letter of the POSIX spec in this detail. TBH, I can't
get excited about that, at least not so far as zic's usage is concerned.
What Windows user is going to be using our version of zic to install
timezone files into a subdirectory that has pre-existing symlinks?

By the same token, I'm pretty unexcited about working around pgreadlink's
deficiency by modifying the IANA code in the way you suggest. It's
painful enough to keep our copy of their code in sync with their updates;
we don't need hacks like that added.

In short, I don't see much of a case for doing anything; but if somebody
were really excited about this they could try to make pgreadlink() fill
the supplied buffer without failing when it's too short.

regards, tom lane