Bugfix: crash when extending an array option by 2+ positions. #1908

lib/Slic3r/GUI/OptionsGroup.pm

@@ -421,12 +421,15 @@ sub _set_config {
     if (defined $index) {
         my $values = $self->config->$get_m($opt_key);
         $values->[$index] = $value;
-        $self->config->set($opt_key, $values);
+        $self->config->set($opt_key, $values)
+            or die "Failed to set $opt_key";
     } else {
         if ($serialized) {
-            return $self->config->set_deserialize($opt_key, $value);
+            return $self->config->set_deserialize($opt_key, $value)
+            or die "Failed to set_deserialize() $opt_key";
         } else {
-            return $self->config->set($opt_key, $value);
+            return $self->config->set($opt_key, $value)
+            or die "Failed to set $opt_key";
         }
     }
 }

lib/Slic3r/GUI/Tab.pm

@@ -733,27 +733,22 @@ sub build {
 sub _extruder_options { qw(nozzle_diameter extruder_offset retract_length retract_lift retract_speed retract_restart_extra retract_before_travel wipe
     retract_layer_change retract_length_toolchange retract_restart_extra_toolchange) }
 
-sub config {
-    my $self = shift;
-    
-    my $config = $self->SUPER::config(@_);
-    
-    # remove all unused values
-    foreach my $opt_key ($self->_extruder_options) {
-        my $values = $config->get($opt_key);
-        splice @$values, $self->{extruders_count} if $self->{extruders_count} <= $#$values;
-        $config->set($opt_key, $values);
-    }
-    
-    return $config;
-}
-
 sub _build_extruder_pages {
     my $self = shift;
     
-    foreach my $extruder_idx (0 .. $self->{extruders_count}-1) {
+    my $default_config = Slic3r::Config::Full->new;
-        # build page if it doesn't exist
+    
-        $self->{extruder_pages}[$extruder_idx] ||= $self->add_options_page("Extruder " . ($extruder_idx + 1), 'funnel.png', optgroups => [
+    foreach my $extruder_idx (@{$self->{extruder_pages}} .. $self->{extruders_count}-1) {
+        # extend options
+        foreach my $opt_key ($self->_extruder_options) {
+            my $values = $self->{config}->get($opt_key);
+            $values->[$extruder_idx] = $default_config->get_at($opt_key, 0);
+            $self->{config}->set($opt_key, $values)
+                or die "Unable to extend $opt_key";
+        }
+        
+        # build page
+        $self->{extruder_pages}[$extruder_idx] = $self->add_options_page("Extruder " . ($extruder_idx + 1), 'funnel.png', optgroups => [
             {
                 title => 'Size',
                 options => ['nozzle_diameter#' . $extruder_idx],
@@ -780,6 +775,19 @@ sub _build_extruder_pages {
         $self->{extruder_pages}[$extruder_idx]{disabled} = 0;
     }
     
+    # remove extra pages
+    if ($self->{extruders_count} <= $#{$self->{extruder_pages}}) {
+        splice @{$self->{extruder_pages}}, $self->{extruders_count};
+    }
+    
+    # remove extra config values
+    foreach my $opt_key ($self->_extruder_options) {
+        my $values = $self->{config}->get($opt_key);
+        splice @$values, $self->{extruders_count} if $self->{extruders_count} <= $#$values;
+        $self->{config}->set($opt_key, $values)
+            or die "Unable to truncate $opt_key";
+    }
+    
     # rebuild page list
     @{$self->{pages}} = (
         (grep $_->{title} !~ /^Extruder \d+/, @{$self->{pages}}),
@@ -793,14 +801,7 @@ sub on_value_change {
     $self->SUPER::on_value_change(@_);
     
     if ($opt_key eq 'extruders_count') {
-        # remove unused pages from list
+        # add extra pages or remove unused
-        my @unused_pages = @{ $self->{extruder_pages} }[$self->{extruders_count} .. $#{$self->{extruder_pages}}];
-        for my $page (@unused_pages) {
-            @{$self->{pages}} = grep $_ ne $page, @{$self->{pages}};
-            $page->{disabled} = 1;
-        }
-        
-        # add extra pages
         $self->_build_extruder_pages;
         
         # update page list and select first page (General)

xs/src/Config.cpp

@@ -180,7 +180,7 @@ ConfigBase::set(t_config_option_key opt_key, SV* value) {
         const size_t len = av_len(av)+1;
         for (size_t i = 0; i < len; i++) {
             SV** elem = av_fetch(av, i, 0);
-            if (!looks_like_number(*elem)) return false;
+            if (elem == NULL || !looks_like_number(*elem)) return false;
             values.push_back(SvNV(*elem));
         }
         optv->values = values;
@@ -193,7 +193,7 @@ ConfigBase::set(t_config_option_key opt_key, SV* value) {
         const size_t len = av_len(av)+1;
         for (size_t i = 0; i < len; i++) {
             SV** elem = av_fetch(av, i, 0);
-            if (!looks_like_number(*elem)) return false;
+            if (elem == NULL || !looks_like_number(*elem)) return false;
             values.push_back(SvIV(*elem));
         }
         optv->values = values;
@@ -205,6 +205,7 @@ ConfigBase::set(t_config_option_key opt_key, SV* value) {
         const size_t len = av_len(av)+1;
         for (size_t i = 0; i < len; i++) {
             SV** elem = av_fetch(av, i, 0);
+            if (elem == NULL) return false;
             optv->values.push_back(std::string(SvPV_nolen(*elem), SvCUR(*elem)));
         }
     } else if (ConfigOptionPoint* optv = dynamic_cast<ConfigOptionPoint*>(opt)) {
@@ -216,7 +217,7 @@ ConfigBase::set(t_config_option_key opt_key, SV* value) {
         for (size_t i = 0; i < len; i++) {
             SV** elem = av_fetch(av, i, 0);
             Pointf point;
-            if (!point.from_SV(*elem)) return false;
+            if (elem == NULL || !point.from_SV(*elem)) return false;
             values.push_back(point);
         }
         optv->values = values;
@@ -228,6 +229,7 @@ ConfigBase::set(t_config_option_key opt_key, SV* value) {
         const size_t len = av_len(av)+1;
         for (size_t i = 0; i < len; i++) {
             SV** elem = av_fetch(av, i, 0);
+            if (elem == NULL) return false;
             optv->values.push_back(SvTRUE(*elem));
         }
     } else {

xs/t/15_config.t

@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use Slic3r::XS;
-use Test::More tests => 100;
+use Test::More tests => 110;
 
 foreach my $config (Slic3r::Config->new, Slic3r::Config::Full->new) {
     $config->set('layer_height', 0.3);
@@ -62,6 +62,11 @@ foreach my $config (Slic3r::Config->new, Slic3r::Config::Full->new) {
     is $config->serialize('extruder_offset'), '10x20,30x45', 'serialize points';
     $config->set_deserialize('extruder_offset', '20x10');
     is_deeply $config->get('extruder_offset'), [[20,10]], 'deserialize points';
+    {
+        my @values = ([10,20]);
+        $values[2] = [10,20];  # implicitely extend array; this is not the same as explicitely assigning undef to second item
+        ok !$config->set('extruder_offset', \@values), 'reject undef points';
+    }
     
     # truncate ->get() to first decimal digit
     $config->set('nozzle_diameter', [0.2,3]);
@@ -71,12 +76,22 @@ foreach my $config (Slic3r::Config->new, Slic3r::Config::Full->new) {
     is_deeply [ map int($_*10)/10, @{$config->get('nozzle_diameter')} ], [0.1,0.4], 'deserialize floats';
     $config->set_deserialize('nozzle_diameter', '3');
     is_deeply [ map int($_*10)/10, @{$config->get('nozzle_diameter')} ], [3], 'deserialize a single float';
+    {
+        my @values = (0.4);
+        $values[2] = 2;  # implicitely extend array; this is not the same as explicitely assigning undef to second item
+        ok !$config->set('nozzle_diameter', \@values), 'reject undef floats';
+    }
     
     $config->set('temperature', [180,210]);
     is_deeply $config->get('temperature'), [180,210], 'set/get ints';
     is $config->serialize('temperature'), '180,210', 'serialize ints';
     $config->set_deserialize('temperature', '195,220');
     is_deeply $config->get('temperature'), [195,220], 'deserialize ints';
+    {
+        my @values = (180);
+        $values[2] = 200;  # implicitely extend array; this is not the same as explicitely assigning undef to second item
+        ok !$config->set('temperature', \@values), 'reject undef ints';
+    }
     
     $config->set('wipe', [1,0]);
     is_deeply $config->get('wipe'), [1,0], 'set/get bools';
@@ -86,12 +101,22 @@ foreach my $config (Slic3r::Config->new, Slic3r::Config::Full->new) {
     is $config->serialize('wipe'), '1,0', 'serialize bools';
     $config->set_deserialize('wipe', '0,1,1');
     is_deeply $config->get('wipe'), [0,1,1], 'deserialize bools';
+    {
+        my @values = (1);
+        $values[2] = 1;  # implicitely extend array; this is not the same as explicitely assigning undef to second item
+        ok !$config->set('wipe', \@values), 'reject undef bools';
+    }
     
     $config->set('post_process', ['foo','bar']);
     is_deeply $config->get('post_process'), ['foo','bar'], 'set/get strings';
     is $config->serialize('post_process'), 'foo;bar', 'serialize strings';
     $config->set_deserialize('post_process', 'bar;baz');
     is_deeply $config->get('post_process'), ['bar','baz'], 'deserialize strings';
+    {
+        my @values = ('foo');
+        $values[2] = 'bar';  # implicitely extend array; this is not the same as explicitely assigning undef to second item
+        ok !$config->set('post_process', \@values), 'reject undef strings';
+    }
     
     is_deeply [ sort @{$config->get_keys} ], [ sort keys %{$config->as_hash} ], 'get_keys and as_hash';
 }