PDP-10 / supdup

Community maintained SUPDUP client for Unix
Other
16 stars 8 forks source link

Unsafe strcpy used on non-OpenBSD systems. #27

Closed johnsonjh closed 1 month ago

johnsonjh commented 3 years ago
supdup.c:
1417:  strcpy(myloc,loc);       /* save */
#ifdef __OpenBSD__
  strlcpy(myloc,loc, sizeof(myloc));        /* save */                
#else
  strcpy(myloc,loc);        /* save */
#endif /* __OpenBSD__ */

~strlcpy is widely supported these days, including on Linux through libbsd.~

~If all else fails, the standard implementation is portable and should be able to be conditionally compiled on most systems without modification.~

~Using $(pkg-config --libs libbsd) to locate the library may be more correct, but adds a pkg-config dependency, obviously, which might not exist on certain embedded systems (and probably not on stock BSD systems).~

I've submitted PR #26 to address this (only tested on Linux).

Edit: Updated to just use the portable implementation, since we probably don't want to do autoconf-ish tests.

johnsonjh commented 3 years ago

Updated to just use the portable version everywhere.

Seems that Travis doesn't have libbsd, so it's likely others won't either.

ams commented 3 years ago

I don't follow. There is no benefit here to use strlcpy? No error checking is done on the return value. You also changed strncpy calls to strlcpy calls?

larsbrinkhoff commented 3 years ago

Certainly doing buffer overflow checks throughout would be a welcome improvement.

johnsonjh commented 3 years ago

Yes, in this particular case there isn't any real change.

However, this does reduce the overall complexity of the code by not requiring operating system specific #ifdef's or using external libraries and eases future maintenance by having a single platform agnostic way to do things.

Also, the replacement of strncpy was:

#ifdef __OpenBSD__
    strlcpy(myloc, argv[1], sizeof(myloc));
#else
    strncpy(myloc, argv[1], sizeof(myloc));
#endif

Using strlcpy on BSD and strncpy and friends on other platforms adds, at a bare minimum, three lines (conditional compilation, no less) for each and every instance of use (not including the duplicated statement itself), while the portable strlcpy implementation is just seven statements in total (not counting variable declarations, comments, and formatting). It has been stable for nearly 25 years and is unlikely to change in the future. Not to mention, with strncpy, there is the need to manually handle NULL-termination in cases of truncation, etc.

Having a single function to use on all platforms that uses the same code, works exactly the same everywhere, and also reduces the total lines of code needed while removing the need for conditional compilation is really the benefit.

(IMO, to this end, the __OpenBSD__ #ifdef's should be removed all together if the change is accepted - I did not do so in this commit to keep the patch to the minimum viable change.)

ams commented 3 years ago

Then I think I'd lke to see the change addresses the entirety of the problem, as it is now I see little benefit. You say that it reduces a few #if's -- but the added code for strlcpy(3) has a size and maintenance cost as well, over 25 years much can happen... :-)

johnsonjh commented 3 years ago

I'll revise it shortly.

johnsonjh commented 1 month ago

Obsoleted