[pve-devel] [PATCH container] fix: cloning a locked container creates an empty config

[pve-devel] [PATCH container] fix: cloning a locked container creates an empty config

[Daniel Tschlatscher]

When an attempt was made to clone a locked container the API would
correctly present the error 'CT is locked (disk)' but create the
config files for the new container anyway and then abort.

The fix is to simply check whether the CT config is locked before
creating the configs for the new container.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/PVE/API2/LXC.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 64724cb..e1b4cd3 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({
 	my $vollist = [];
 	my $running;
 
-	PVE::LXC::Config->create_and_lock_config($newid, 0);
-	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
-
 	my $lock_and_reload = sub {
 	    my ($vmid, $code) = @_;
 	    return PVE::LXC::Config->lock_config($vmid, sub {
@@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({
 
 	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
 
+	PVE::LXC::Config->create_and_lock_config($newid, 0);
+	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
+
 	$running = PVE::LXC::check_running($vmid) || 0;
 
 	my $full = extract_param($param, 'full');
-- 
2.30.2

Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config

[Fabian Grünbichler]

On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote:
> When an attempt was made to clone a locked container the API would
> correctly present the error 'CT is locked (disk)' but create the
> config files for the new container anyway and then abort.
> 
> The fix is to simply check whether the CT config is locked before
> creating the configs for the new container.

is there a reason for not just moving it to the start of the eval block 
to avoid the same problem being re-introduced in the future? any error 
occuring inside the eval block will then trigger a cleanup..

> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 64724cb..e1b4cd3 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({
>  	my $vollist = [];
>  	my $running;
>  
> -	PVE::LXC::Config->create_and_lock_config($newid, 0);
> -	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
> -
>  	my $lock_and_reload = sub {
>  	    my ($vmid, $code) = @_;
>  	    return PVE::LXC::Config->lock_config($vmid, sub {
> @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({
>  
>  	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
>  
> +	PVE::LXC::Config->create_and_lock_config($newid, 0);
> +	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
> +
>  	$running = PVE::LXC::check_running($vmid) || 0;
>  
>  	my $full = extract_param($param, 'full');
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config

[Daniel Tschlatscher]

On 6/14/22 14:51, Fabian Grünbichler wrote:
> On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote:
>> When an attempt was made to clone a locked container the API would
>> correctly present the error 'CT is locked (disk)' but create the
>> config files for the new container anyway and then abort.
>>
>> The fix is to simply check whether the CT config is locked before
>> creating the configs for the new container.
> 
> is there a reason for not just moving it to the start of the eval block 
> to avoid the same problem being re-introduced in the future? any error 
> occuring inside the eval block will then trigger a cleanup..
> 

When an error occurs and the cleanup is triggered, the cleanup tries to
release the lock again.

Moving the set_lock function into the eval block would create a problem
here:
If the lock was created by another process (and if it is a 'disk' lock)
set_lock would emit an error and the lock would be incorrectly released
by this process, which did not originally acquire it.

>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>> ---
>>  src/PVE/API2/LXC.pm | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index 64724cb..e1b4cd3 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({
>>  	my $vollist = [];
>>  	my $running;
>>  
>> -	PVE::LXC::Config->create_and_lock_config($newid, 0);
>> -	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>> -
>>  	my $lock_and_reload = sub {
>>  	    my ($vmid, $code) = @_;
>>  	    return PVE::LXC::Config->lock_config($vmid, sub {
>> @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({
>>  
>>  	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
>>  
>> +	PVE::LXC::Config->create_and_lock_config($newid, 0);
>> +	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>> +
>>  	$running = PVE::LXC::check_running($vmid) || 0;
>>  
>>  	my $full = extract_param($param, 'full');
>> -- 
>> 2.30.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config

[Fabian Grünbichler]

On June 14, 2022 3:43 pm, Daniel Tschlatscher wrote:
> 
> 
> On 6/14/22 14:51, Fabian Grünbichler wrote:
>> On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote:
>>> When an attempt was made to clone a locked container the API would
>>> correctly present the error 'CT is locked (disk)' but create the
>>> config files for the new container anyway and then abort.
>>>
>>> The fix is to simply check whether the CT config is locked before
>>> creating the configs for the new container.
>> 
>> is there a reason for not just moving it to the start of the eval block 
>> to avoid the same problem being re-introduced in the future? any error 
>> occuring inside the eval block will then trigger a cleanup..
>> 
> 
> When an error occurs and the cleanup is triggered, the cleanup tries to
> release the lock again.
> 
> Moving the set_lock function into the eval block would create a problem
> here:
> If the lock was created by another process (and if it is a 'disk' lock)
> set_lock would emit an error and the lock would be incorrectly released
> by this process, which did not originally acquire it.

my suggestion was not to move it *into* the eval block, but next to the 
*start* of the eval block, like so:

	my $newconf = {};
	my $mountpoints = {};
	my $fullclone = {};
	my $vollist = [];
	my $running;

	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');

	$running = PVE::LXC::check_running($vmid) || 0;

	my $full = extract_param($param, 'full');
	if (!defined($full)) {
	    $full = !PVE::LXC::Config->is_template($src_conf);
	}

	my $lock_and_reload = sub {
	    my ($vmid, $code) = @_;
	    return PVE::LXC::Config->lock_config($vmid, sub {
		my $conf = PVE::LXC::Config->load_config($vmid);
		die "Lost 'create' config lock, aborting.\n"
		    if !PVE::LXC::Config->has_lock($conf, 'create');
		return $code->($conf);
	    });
	};

	PVE::LXC::Config->create_and_lock_config($newid, 0);
	PVE::Firewall::clone_vmfw_conf($vmid, $newid);

  # error handling for this block will cleanup configs
	eval {
	    die "parameter 'storage' not allowed for linked clones\n"
		if defined($storage) && !$full;


to reduce the chances of re-introducing the bug by adding potentially 
failing calls between create_and_lock_config and the eval block. and now 
that I took a second look I realized that clone_vm_fw_conf is exactly 
such a call, and should therefore be moved into the eval ;)

> 
>>>
>>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>>> ---
>>>  src/PVE/API2/LXC.pm | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>>> index 64724cb..e1b4cd3 100644
>>> --- a/src/PVE/API2/LXC.pm
>>> +++ b/src/PVE/API2/LXC.pm
>>> @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({
>>>  	my $vollist = [];
>>>  	my $running;
>>>  
>>> -	PVE::LXC::Config->create_and_lock_config($newid, 0);
>>> -	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>>> -
>>>  	my $lock_and_reload = sub {
>>>  	    my ($vmid, $code) = @_;
>>>  	    return PVE::LXC::Config->lock_config($vmid, sub {
>>> @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({
>>>  
>>>  	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
>>>  
>>> +	PVE::LXC::Config->create_and_lock_config($newid, 0);
>>> +	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>>> +
>>>  	$running = PVE::LXC::check_running($vmid) || 0;
>>>  
>>>  	my $full = extract_param($param, 'full');
>>> -- 
>>> 2.30.2
>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>>
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
>> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>