[ipxe-devel] [PATCH] [efi] use correct bound in strncpy to ensure NUL-termination
brogers at suse.com
Thu May 3 18:43:45 BST 2018
>>> 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
>> 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
>> (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
>> 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.
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.
More information about the ipxe-devel