[libvirt] [PATCH] [libvirt-php] fix libvirt_stream_send/libvirt_stream_recv

[Michal Privoznik]

On 03/29/2017 07:51 PM, Vasiliy Tolstov wrote:
> Signed-off-by: Vasiliy Tolstov <v.tolstov at selfip.ru>
> ---
>  src/libvirt-php.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> index e8384795d635..4e088e746281 100644
> --- a/src/libvirt-php.c
> +++ b/src/libvirt-php.c
> @@ -178,9 +178,15 @@ ZEND_ARG_INFO(0, stats)
>  ZEND_ARG_INFO(0, flags)
>  ZEND_END_ARG_INFO()
>
> -ZEND_BEGIN_ARG_INFO_EX(arginfo_libvirt_stream_send, 0, 0, 2)
> +ZEND_BEGIN_ARG_INFO_EX(arginfo_libvirt_stream_send, 0, 1, 2)
>  ZEND_ARG_INFO(0, conn)
> -ZEND_ARG_INFO(0, data)
> +ZEND_ARG_INFO(1, data)
> +ZEND_ARG_INFO(0, len)
> +ZEND_END_ARG_INFO()
> +
> +ZEND_BEGIN_ARG_INFO_EX(arginfo_libvirt_stream_recv, 0, 1, 2)
> +ZEND_ARG_INFO(0, conn)
> +ZEND_ARG_INFO(1, data)
>  ZEND_ARG_INFO(0, len)
>  ZEND_END_ARG_INFO()
>
> @@ -595,7 +601,7 @@ static zend_function_entry libvirt_functions[] = {
>      PHP_FE(libvirt_stream_abort,                 arginfo_libvirt_conn)
>      PHP_FE(libvirt_stream_finish,                arginfo_libvirt_conn)
>      PHP_FE(libvirt_stream_send,                  arginfo_libvirt_stream_send)
> -    PHP_FE(libvirt_stream_recv,                  arginfo_libvirt_stream_send)
> +    PHP_FE(libvirt_stream_recv,                  arginfo_libvirt_stream_recv)
>      /* Domain functions */
>      PHP_FE(libvirt_domain_new,                   arginfo_libvirt_domain_new)
>      PHP_FE(libvirt_domain_new_get_vnc,           arginfo_libvirt_void)
> @@ -4509,7 +4515,7 @@ PHP_FUNCTION(libvirt_stream_finish)
>  /*
>   * Function name:   libvirt_stream_recv
>   * Since version:   0.5.0
> - * Description:     Function is used to close stream from libvirt conn
> + * Description:     Function is used to recv from stream via libvirt conn
>   * Arguments:       @res [resource]: libvirt stream resource from libvirt_stream_create()
>   *                  @data [string]: buffer
>   *                  @len [int]: amout of data to recieve
> @@ -4518,8 +4524,8 @@ PHP_FUNCTION(libvirt_stream_finish)
>  PHP_FUNCTION(libvirt_stream_recv)
>  {
>      zval *zstream, *zbuf;
> -    char *recv_buf;
>      php_libvirt_stream *stream = NULL;
> +    char *recv_buf = NULL;
>      int retval = -1;
>      zend_long length = 0;
>
> @@ -4534,31 +4540,24 @@ PHP_FUNCTION(libvirt_stream_recv)

There's memset() just above this line. It is completely useless - 
because the recv_buf will be overwritten straight away. I'm appending 
its removal to this patch.

>
>      retval = virStreamRecv(stream->stream, recv_buf, length);
>      if (retval < 0) {
> -        efree(recv_buf);
> -        zval_dtor(zbuf);
> -        ZVAL_NULL(zbuf);
> +      zval_dtor(zbuf);
> +      ZVAL_NULL(zbuf);

This space change does not look right.

>      } else {
>          recv_buf[retval] = '\0';

Frankly, I've no idea whether this should be here. I feel like no 
because VIRT_ZVAL_STRINGL should not look into the recv_buf at any 
position greater than retval. That is why we give it the length of the 
string after all. Can you please check whether your case still works 
after this line is removed?

> -#if PHP_MAJOR_VERSION >= 7
> -        ZVAL_STRINGL(zbuf, recv_buf, retval);
> -        efree(recv_buf);
> -#else
> -        ZVAL_STRINGL(zbuf, recv_buf, retval, 0);
> -#endif
> +        VIRT_ZVAL_STRINGL(zbuf, recv_buf, retval);
>      }
>
>      if (retval == -1) {
>          set_error("Cannot recv from stream" TSRMLS_CC);
> -        RETURN_LONG(retval);
>      }
> -
> +    efree(recv_buf);
>      RETURN_LONG(retval);
>  }
>
>  /*
>   * Function name:   libvirt_stream_send
>   * Since version:   0.5.0
> - * Description:     Function is used to close stream from libvirt conn
> + * Description:     Function is used to send to stream via libvirt conn
>   * Arguments:       @res [resource]: libvirt stream resource from libvirt_stream_create()
>   *                  @data [string]: buffer
>   *                  @length [int]: amout of data to send
> @@ -4571,7 +4570,6 @@ PHP_FUNCTION(libvirt_stream_send)
>      int retval = -1;
>      zend_long length = 0;
>      char *cstr;
> -    //int cstrlen;
>
>      if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rz|l", &zstream, &zbuf, &length) == FAILURE)
>          RETURN_LONG(retval);
> @@ -4580,12 +4578,10 @@ PHP_FUNCTION(libvirt_stream_send)
>          RETURN_LONG(retval);
>
>      cstr = Z_STRVAL_P(zbuf);
> -    //cstrlen = Z_STRLEN_P(zbuf);
>
>      retval = virStreamSend(stream->stream, cstr, length);
>      if (retval == -1) {
>          set_error("Cannot send to stream" TSRMLS_CC);
> -        RETURN_LONG(retval);
>      }

I'm dropping these curly braces around one line body.

>
>      RETURN_LONG(retval);
>

ACKed and pushed.

Michal