[Python-Dev] [Python-checkins] cpython: Make sure that *really* no more than sizeof(ifr.ifr_name) chars are strcpy-ed

[Stefan Krah]

christian.heimes <python-checkins at python.org> wrote:
>   Make sure that *really* no more than sizeof(ifr.ifr_name) chars are strcpy-ed to ifr.ifr_name and that the string is *always* NUL terminated. New code shouldn't use strcpy(), too. CID 719692
> 
> files:
>   Modules/socketmodule.c |  3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
> --- a/Modules/socketmodule.c
> +++ b/Modules/socketmodule.c
> @@ -1674,7 +1674,8 @@
>              if (len == 0) {
>                  ifr.ifr_ifindex = 0;
>              } else if (len < sizeof(ifr.ifr_name)) {
> -                strcpy(ifr.ifr_name, PyBytes_AS_STRING(interfaceName));
> +                strncpy(ifr.ifr_name, PyBytes_AS_STRING(interfaceName), sizeof(ifr.ifr_name));
> +                ifr.ifr_name[(sizeof(ifr.ifr_name))-1] = '\0';
>                  if (ioctl(s->sock_fd, SIOCGIFINDEX, &ifr) < 0) {
>                      s->errorhandler();
>                      Py_DECREF(interfaceName);


I've trouble finding the overrun in the existing code. Previously:

We have:

    1) len = PyBytes_GET_SIZE(interfaceName); (without NUL terminator)


At the point of strcpy() we have:

    2) len < sizeof(ifr.ifr_name)

PyBytes_AS_STRING(interfaceName) always adds a NUL terminator, so:

    3) len+1 <= sizeof(ifr.ifr_name)

    4) strcpy(ifr.ifr_name, PyBytes_AS_STRING(interfaceName)) will not overrun
       ifr.ifr_name and ifr.ifr_name is always NUL terminated.


So IMO the strcpy() was safe and the report is a false positive.


Stefan Krah