0

Background: I have a webapp that uses uploadprogress and it just caused a segmentation fault. Investigating about the problem I found a bug report that points the problem down to a line in the code https://bugs.php.net/bug.php?id=79584

In this code I also saw, that they use strcpy. The function is used there for many years already so I guess it has been checked against security rules, but I am not a C programmer and my knowledge is very limited. I also found mentions that it sometimes can be legitimate to still use strcpy without causing any security issues.

Now I wonder if that's the case in this special package. Is it safe that the code contains this function or not?

Mike Kinghan
  • 55,740
  • 12
  • 153
  • 182
user6329530
  • 596
  • 1
  • 7
  • 21
  • 9
    `strcpy` is perfectly safe if you ensure that the destination buffer is large enough for the string to be copied. – Jabberwocky May 12 '20 at 14:16
  • I prefer `strncpy` all the time – Muhammedogz May 12 '20 at 14:21
  • 1
    `strcpy` is a C function. C functions are *supposed* to be dangerous. That's what makes C programming so much fun. – Adrian Mole May 12 '20 at 14:22
  • Possible duplicate: [C4996 (function unsafe) warning for strcpy but not for memcpy](https://stackoverflow.com/questions/23486938/c4996-function-unsafe-warning-for-strcpy-but-not-for-memcpy). – Lundin May 12 '20 at 14:23
  • 7
    @muhammedoğuz but you are aware that `strncpy` may leave you with a non NUL terminated sequence of chars – Jabberwocky May 12 '20 at 14:23
  • 4
    @muhammedoğuz Unlike `strcpy`, `strncpy` is a dangerous function that should be avoided. Check my answer here to [Which functions from the standard library must (should) be avoided?](https://stackoverflow.com/a/46563868/584518). – Lundin May 12 '20 at 14:26
  • 1
    Before the doggy change introduced by that [commit](https://github.com/php/pecl-php-uploadprogress/commit/e12376f7fd51e386aa8c9be922732e389c1eee7a), the use of `strcpy` looked safe. The destination buffer was allocated dynamically using the length of the source string plus one as its size, so is large enough for the `strcpy` call. (The commit breaks it horribly.) – Ian Abbott May 12 '20 at 14:36
  • @IanAbbott thanks for clarification – user6329530 May 12 '20 at 14:49

1 Answers1

3

The bug produced by that commit has absolutely nothing to do with the safety of strcpy (or lack thereof).

strcpy is not safe unless you have some way to ensure that the destination object is big enough to hold the source (including its NUL terminator). If you do that, then it is as safe as your checks are.

Other than the confusion with dereference operators, the methodology of the linked code is adequate. Measuring strlen of the source string and adding one is a sufficient method to determine the length required; malloc of this amount is a sufficient method for allocating this much space. To that extent, strcpy(malloc(strlen(src) + 1), src) is as safe as you're going to find in C (aside from the cheekiness of not testing the return code of malloc for NULL; in real production code, you should do that, or use a malloc wrapper like emalloc in the PHP source).

However, strdup(src) is much more concise and readable, and its conciseness makes typos less probable. Personally, I would recommend to always use strdup for such cases; if you are worried that your target platform doesn't contain an implementation, it's easy enough to shim.

rici
  • 234,347
  • 28
  • 237
  • 341
  • "The bug produced by that commit has absolutely nothing to do with the safety of strcpy" I know. I didn't want to suggest that it does. It's just something I saw when reading the bug report and wondered if there might be a(nother) issue. – user6329530 May 12 '20 at 15:07
  • 1
    @supertyp: I just wanted to say that, since there is a class of people who are always looking for evidence that *X* is unsafe, for various values of *X*. That particular bug would be just as buggy with any `strcpy` replacement, except of course the replacement I suggest :-) where it wouldn't be possible. I added something to make the answer more explicit. – rici May 12 '20 at 15:11