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

[Michael Brown]

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.

Thanks,

Michael