Re: [gnustep/libs-gui] Named image unarchiving (#12)

[Josh Freeman]

Hi,

   This patch (which has now been merged to the trunk) looks like it  
will cause a crash if trying to load an unknown named-image from a  
keyed archive:

-[NSImage initWithCoder:] (NSImage.m, lines 1976-1998):
----
      if ([coder containsValueForKey: @"NSName"])
        {
          RELEASE(self);
          imageName = [coder decodeObjectForKey: @"NSName"];
          replacementImage = [NSImage imageNamed: imageName];
          if (replacementImage)
            {
              return RETAIN(replacementImage);
            }
          replacementImage = [[NSImage alloc] init];
          [replacementImage setName: imageName];
          replacementImage->_flags.archiveByName = YES;
        }
----

   self is released in the second line (probably deallocating it),  
but if the named image isn't found ([NSImage imageNamed: imageName]  
returns nil), control passes through the rest of the above scope, to  
the end of the method, which returns (the now-deallocated) self. (The  
new image that's allocated to replace the original instance is stored  
in replacementImage instead of self, and isn't accessed again).

   One fix would be to store the reallocated image in self instead of  
replacementImage (which is how it's done in the non-keyed archive  
logic, later in the same method); Alternatively, I'd suggest delaying  
the release of self until after verifying that the named image was  
found, which avoids having to reallocate if it falls through:

----
      if ([coder containsValueForKey: @"NSName"])
        {
          imageName = [coder decodeObjectForKey: @"NSName"];
          replacementImage = [NSImage imageNamed: imageName];
          if (replacementImage)
            {
              RELEASE(self);
              return RETAIN(replacementImage);
            }
          [self setName: theName];
          self->_flags.archiveByName = YES;
        }
----


   Another thing regarding -[NSImage initWithCoder:] (unrelated to  
the patch): Is it missing a 'self = [self init];' at the beginning?

Cheers,

Josh



On Oct 28, 2017, at 3:48 AM, Graham Lee wrote:

> 2017-10-28 Graham Lee graham@iamleeg.com
>
> * Source/NSImage.m (initWithCoder:): restore image name when
> loading an archived image with a name that can't be resolved at
> load time. Fixes Gorm bug #46317.
> You can view, comment on, or merge this pull request online at:
>   https://github.com/gnustep/libs-gui/pull/12
>
> Commit Summary
>         • If an unknown named image is unarchived, keep the name
>         • revert spurious whitespace change
> File Changes
>         • M Source/NSImage.m (27)
> Patch Links:
>         • https://github.com/gnustep/libs-gui/pull/12.patch
>         • https://github.com/gnustep/libs-gui/pull/12.diff