[LinuxCNC/linuxcnc PR#495] Reduce use of sprintf, strcpy and strcat About GRBL HAL

Issue #495 | 状态: 已关闭 | 作者: jepler | 创建时间: 2018-09-13

These functions are generally regarded as unsafe. There are many opinions on what to do about this, but this patchset takes the viewpoint that replacing them with truncating versions is almost certain to give better results.

This patchset
* Introduces rtapistrlcpy, rtapistrlcat with semantics like bsd strlcpy/strlcat
* Introduces rtapistrxcpy, rtapistrxcat with parameter lists like strcpy and strcat, but which check that the destination is an array and use the destination array’s size automatically with rtapi_strl{cpy,cat}. Use with a non-array is a compile-time error.
* Documents the lot of them in man 3rtapi rtapi_strlcpy
* Change all amenable uses of sprintf -> snprintf, strcpy -> rtapistrxcpy, strcat ->rtapistrxcat
* Enables a future patch to fix the remaining (non-array) uses of strcpy/strcat to truncating strlcpy/strlcat, just requiring someone to carry through the allocation sizes to the call sites

This ranges widely over the source tree, let me know if a chopped up version would be easier to accept.

Testing performed: scripts/runtests on a stretch uspace system is in progress but I wanted to create this pull request before calling it a night.

#1 – jepler 于 2018-09-13

urk. Unfortunately, the ‘header sanity’ test which failed on travis succeeds locally, and no further information is provided :frowning:

#2 – jepler 于 2018-09-13

I suspect it’s because of the use of C++11 features in string.h>. Pushed a fresh commit to address that and we’re off to the races again. If that works out I’ll rebase so that commit is first. However, this means a little further soul searching is necessary to accept this commit: we’ve just made string.h> require C++11 support when using C++, which was not true of any of our headers before now. (C(99) works fine for this header too)

#3 – jepler 于 2018-09-13

But according to our documentation at rtapistring.3rtapi the functions in this header have only been for kernel space before now, despite that including the header worked just fine in userspace. In practice, only the definition of rtapikstrdup, which is a #define to strdup would have worked.

#4 – jepler 于 2018-09-14

Now it builds clean on all architectures tested by buildbot.

#5 – jepler 于 2018-11-04

Rebased to resolve merge conflicts with emcrsh due to multispindle changes.

#6 – andypugh 于 2020-04-09

gcc-8 has started complaining about possible truncation using strncpy etc. I think we should probably re-visit this patch for master. (it’s possibly just a bit too big for 2.8 at this point)

#7 – rene-dev 于 2020-04-09

I disagree, it doesnt change functionality, and should not cause problems in 2.8.
If no one else wants to do it, I can take care of it this week.

#8 – andypugh 于 2020-04-09

I just resolved the merge conflicts, at least.

#9 – andypugh 于 2020-04-09

Though can I invite you to take a look at https://github.com/LinuxCNC/linuxcnc/blob/master/src/hal/classicladder/manager_gtk.c#L119
Where I think that c1f6438155c9d642ac221300424f7326f04a3d53 might have been a bit over-zealous.

#10 – rene-dev 于 2020-04-20

all cherrypicked into 2.8, and master.
thanks!

原始Issue: https://github.com/LinuxCNC/linuxcnc/pull/495

评论 (10)