[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.

As an additional follow up, now that gcc8 is released, I decided on the
following patch for us in the SUSE/openSUSE qemu package, which also
builds ipxe from source.

https://build.opensuse.org/package/view_file/openSUSE:Factory/qemu/ipxe-efi-guard-strncpy-with-gcc-warning-ignore-pragma.patch?expand=1

As you can see, it simply guards this one offending instance of strncpy()
with GCC specific macros to avoid the warning. I didn't think this would
be the solution you would want for the upstream ipxe codebase, so I
didn't send this patch to the mailing list.

Bruce