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

Vasiliy Tolstov posted 1 patch 6 years, 12 months ago
Failed in applying to current master (apply log)
src/libvirt-php.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
[libvirt] [PATCH] [libvirt-php] fix libvirt_stream_send/libvirt_stream_recv
Posted by Vasiliy Tolstov 6 years, 12 months ago
Signed-off-by: Vasiliy Tolstov <v.tolstov@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)
 
     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);
     } else {
         recv_buf[retval] = '\0';
-#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);
     }
 
     RETURN_LONG(retval);
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] [libvirt-php] fix libvirt_stream_send/libvirt_stream_recv
Posted by Michal Privoznik 6 years, 12 months ago
On 03/29/2017 07:51 PM, Vasiliy Tolstov wrote:
> Signed-off-by: Vasiliy Tolstov <v.tolstov@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] [libvirt-php] fix libvirt_stream_send/libvirt_stream_recv
Posted by Vasiliy Tolstov 6 years, 12 months ago
2017-03-30 16:06 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
> 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?


I'm try raw images and  recv_buf[retval] = '\0'; does not bring any
problems (md5sum on source and dest equal with and without this).
But can you look into send function in this function we convert zval
to C string. As i understand leading zero dropped in this case ?

-- 
Vasiliy Tolstov,
e-mail: v.tolstov@selfip.ru

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] [libvirt-php] fix libvirt_stream_send/libvirt_stream_recv
Posted by Michal Privoznik 6 years, 12 months ago
On 03/30/2017 03:18 PM, Vasiliy Tolstov wrote:
> 2017-03-30 16:06 GMT+03:00 Michal Privoznik <mprivozn@redhat.com>:
>> 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?
>
>
> I'm try raw images and  recv_buf[retval] = '\0'; does not bring any
> problems (md5sum on source and dest equal with and without this).
> But can you look into send function in this function we convert zval
> to C string. As i understand leading zero dropped in this case ?
>

I guess so. So if you want to drop the line feel free to propose the 
patch. If you don't feel like touching the code - that is okay too.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list