Ranier Vilela <ranier...@gmail.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