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

["Denis V. Lunev" <den@virtuozzo.com>]

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