[PATCH 1/1] qemu-nbd: regression with arguments passing into nbd_client_thread()

[PATCH 1/1] qemu-nbd: regression with arguments passing into nbd_client_thread()

[Denis V. Lunev]

Unfortunately
    commit 03b67621445d601c9cdc7dfe25812e9f19b81488
    Author: Denis V. Lunev <den@openvz.org>
    Date:   Mon Jul 17 16:55:40 2023 +0200
    qemu-nbd: pass structure into nbd_client_thread instead of plain char*
has introduced a regression. struct NbdClientOpts resides on stack inside
'if' block. This specifically means that this stack space could be reused
once the execution will leave that block of the code.

This means that parameters passed into nbd_client_thread could be
overwritten at any moment.

The patch moves the data to the namespace of main() function effectively
preserving it for the whole process lifetime.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Eric Blake <eblake@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: <qemu-stable@nongnu.org>
---
 qemu-nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 5b2757920c..7a15085ade 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -589,6 +589,7 @@ int main(int argc, char **argv)
     const char *pid_file_name = NULL;
     const char *selinux_label = NULL;
     BlockExportOptions *export_opts;
+    struct NbdClientOpts opts;
 
 #ifdef CONFIG_POSIX
     os_setup_early_signal_handling();
@@ -1145,7 +1146,7 @@ int main(int argc, char **argv)
     if (device) {
 #if HAVE_NBD_DEVICE
         int ret;
-        struct NbdClientOpts opts = {
+        opts = (struct NbdClientOpts) {
             .device = device,
             .fork_process = fork_process,
             .verbose = verbose,
-- 
2.34.1

Re: [PATCH 1/1] qemu-nbd: regression with arguments passing into nbd_client_thread()

[Eric Blake]

On Wed, Jul 26, 2023 at 04:52:47PM +0200, Denis V. Lunev wrote:
> Unfortunately
>     commit 03b67621445d601c9cdc7dfe25812e9f19b81488
>     Author: Denis V. Lunev <den@openvz.org>
>     Date:   Mon Jul 17 16:55:40 2023 +0200
>     qemu-nbd: pass structure into nbd_client_thread instead of plain char*
> has introduced a regression. struct NbdClientOpts resides on stack inside
> 'if' block. This specifically means that this stack space could be reused
> once the execution will leave that block of the code.
> 
> This means that parameters passed into nbd_client_thread could be
> overwritten at any moment.
> 
> The patch moves the data to the namespace of main() function effectively
> preserving it for the whole process lifetime.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> CC: <qemu-stable@nongnu.org>
> ---
>  qemu-nbd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 5b2757920c..7a15085ade 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -589,6 +589,7 @@ int main(int argc, char **argv)
>      const char *pid_file_name = NULL;
>      const char *selinux_label = NULL;
>      BlockExportOptions *export_opts;
> +    struct NbdClientOpts opts;
>  
>  #ifdef CONFIG_POSIX
>      os_setup_early_signal_handling();
> @@ -1145,7 +1146,7 @@ int main(int argc, char **argv)
>      if (device) {
>  #if HAVE_NBD_DEVICE
>          int ret;
> -        struct NbdClientOpts opts = {
> +        opts = (struct NbdClientOpts) {

Does this case a compiler warning for an unused variable when
HAVE_NBD_DEVICE is not set?  If so, the solution is to also wrap the
declaration in the same #if.  I'll see if I can figure out the CI
enough to prove (or disprove) my theory on a BSD machine which likely
lacks HAVE_NBD_DEVICE.

With that addressed, I'm fine with:

Reviewed-by: Eric Blake <eblake@redhat.com>

and I will queue it through my NBD tree in time for 8.1-rc2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 1/1] qemu-nbd: regression with arguments passing into nbd_client_thread()

[Denis V. Lunev]

On 7/26/23 19:57, Eric Blake wrote:
> On Wed, Jul 26, 2023 at 04:52:47PM +0200, Denis V. Lunev wrote:
>> Unfortunately
>>      commit 03b67621445d601c9cdc7dfe25812e9f19b81488
>>      Author: Denis V. Lunev <den@openvz.org>
>>      Date:   Mon Jul 17 16:55:40 2023 +0200
>>      qemu-nbd: pass structure into nbd_client_thread instead of plain char*
>> has introduced a regression. struct NbdClientOpts resides on stack inside
>> 'if' block. This specifically means that this stack space could be reused
>> once the execution will leave that block of the code.
>>
>> This means that parameters passed into nbd_client_thread could be
>> overwritten at any moment.
>>
>> The patch moves the data to the namespace of main() function effectively
>> preserving it for the whole process lifetime.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> CC: <qemu-stable@nongnu.org>
>> ---
>>   qemu-nbd.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 5b2757920c..7a15085ade 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -589,6 +589,7 @@ int main(int argc, char **argv)
>>       const char *pid_file_name = NULL;
>>       const char *selinux_label = NULL;
>>       BlockExportOptions *export_opts;
>> +    struct NbdClientOpts opts;
>>   
>>   #ifdef CONFIG_POSIX
>>       os_setup_early_signal_handling();
>> @@ -1145,7 +1146,7 @@ int main(int argc, char **argv)
>>       if (device) {
>>   #if HAVE_NBD_DEVICE
>>           int ret;
>> -        struct NbdClientOpts opts = {
>> +        opts = (struct NbdClientOpts) {
> Does this case a compiler warning for an unused variable when
> HAVE_NBD_DEVICE is not set?  If so, the solution is to also wrap the
> declaration in the same #if.  I'll see if I can figure out the CI
> enough to prove (or disprove) my theory on a BSD machine which likely
> lacks HAVE_NBD_DEVICE.
>
> With that addressed, I'm fine with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> and I will queue it through my NBD tree in time for 8.1-rc2.
>
Double checked. We will have an error there as 'struct NbdClientOps'
is defined under HAVE_NBD_DEVICE only.

../qemu-nbd.c: In function ‘main’:
../qemu-nbd.c:592:26: error: storage size of ‘opts’ isn’t known
   592 |     struct NbdClientOpts opts;
       |                          ^~~~
../qemu-nbd.c:592:26: warning: unused variable ‘opts’ [-Wunused-variable]

Checked with simple
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -52,7 +52,7 @@
  #endif

  #ifdef __linux__
-#define HAVE_NBD_DEVICE 1
+#define HAVE_NBD_DEVICE 0
  #else
  #define HAVE_NBD_DEVICE 0
  #endif

Den