[ipxe-devel] [PATCH] [efi] use correct bound in strncpy to ensure NUL-termination

[Bruce Rogers]

>>> On 4/23/2018 at 2:30 PM, Michael Brown <mcb30 at ipxe.org> wrote:
> On 23/04/18 21:01, Bruce Rogers wrote:
>>> This is a fixed-length string field that is not supposed to be
>>> NUL-terminated.  The use of strncpy() here is deliberate in order to be
>>> able to completely fill the field.
>>>
>>> Is there a (clean) way to indicate to gcc to ignore this false positive
>>> warning?
>> 
>> I don't know if there is a way to do that.
>> 
>> It looks like this is not the only instance of this type of usage of 
> strncpy()
>> (I should have checked). This new check in gcc8 also covers strncat() and
>> stpncpy(). I find one instance of strncat(), but it's usage looks fine. No 
> usage
>> of stpncpy().  Perhaps we need an audit of all these strncpy()'s to properly
>> avoid this issue as gcc8 is coming soon.
> 
>  From a very quick review of the instances of strncpy(), all appear to 
> be deliberate.  They are all used to copy a NUL-terminated source string 
> into a non-NUL-terminated fixed-size buffer that requires NUL-padding. 
> This is precisely what strncpy() is defined to do (and we have unit 
> tests to ensure that this is what our implementation does!).
> 
> Note that using memcpy() alone is insufficient, since you would need a 
> separate calculation to determine the correct length to be copied (which 
> depends on both the source string and the destination buffer size) and 
> also a call to memset() to perform the NUL-padding (in those cases where 
> the destination buffer is not already guaranteed to be initialised to 
> all NULs).
> 
> We could reimplement each instance using a call to strlen(), an explicit 
> check to ensure that the destination length is not exceeded, a call to 
> memcpy(), and a (possible) call to memset() to perform the padding. 
> This would basically be reimplementing strncpy() at each call site, 
> which is not a sensible solution.
> 
> I'll happily accept patches to alter gcc's behaviour so that it does not 
> report false positive warnings for these 100% correct usages of 
> strncpy().  There may be a clean way to do this; if not then I'll accept 
> an alternative patch to disable stringop-truncation warnings completely.

The changes webpage for gcc8 at https://gcc.gnu.org/gcc-8/changes.html 
indicates a way to suppress the warning, but given that is based on a bug,
which may get fixed, I don't know that that is a good approach. For the
instance I've pointed out, I don't know of a way to not report that false
positive.

I'll submit a patch to disable the stringop-truncation warnings completely
for your consideration. Hopefully a better approach can be found.

Bruce