Skip to content

Commit

Permalink
Fix flag handling and storage type validation to match
Browse files Browse the repository at this point in the history 
Long name attribute flags was not being set correctly and the rules
around when there is a mismatch between the bit flags in the header and
the type attribute were not being correctly handled

Signed-off-by: Kimball Thurston <[email protected]>
  • Loading branch information
@kdt3rd
kdt3rd committed Apr 20, 2024
1 parent f38fb9d commit e7f1099
Show file tree
Hide file tree
Showing 6 changed files with 166 additions and 57 deletions.
14 changes: 6 additions & 8 deletions src/lib/OpenEXRCore/context.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,17 +395,15 @@ exr_get_file_version_and_flags (exr_const_context_t ctxt, uint32_t* ver)

if (ver)
{
uint32_t flags = ctxt->version;
exr_result_t ret = EXR_ERR_SUCCESS;

if (ctxt->is_multipart) flags |= EXR_MULTI_PART_FLAG;
if (ctxt->max_name_length > EXR_SHORTNAME_MAXLEN)
flags |= EXR_LONG_NAMES_FLAG;
if (ctxt->has_nonimage_data) flags |= EXR_NON_IMAGE_FLAG;
if (ctxt->is_singlepart_tiled) flags |= EXR_TILED_FLAG;
if (ctxt->orig_version_and_flags != 0)
*ver = ctxt->orig_version_and_flags;
else
ret = internal_exr_calc_header_version_flags (ctxt, ver);

*ver = flags;
if (ctxt->mode == EXR_CONTEXT_WRITE) internal_exr_unlock (ctxt);
return EXR_ERR_SUCCESS;
return ret;
}

if (ctxt->mode == EXR_CONTEXT_WRITE) internal_exr_unlock (ctxt);
Expand Down
1 change: 1 addition & 0 deletions src/lib/OpenEXRCore/internal_file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ exr_result_t internal_exr_compute_tile_information (
exr_context_t ctxt, exr_priv_part_t curpart, int rebuild);
int32_t internal_exr_compute_chunk_offset_size (exr_priv_part_t curpart);

exr_result_t internal_exr_calc_header_version_flags (exr_const_context_t ctxt, uint32_t *flags);
exr_result_t internal_exr_write_header (exr_context_t ctxt);

/* in openexr_validate.c, functions to validate the header during read / pre-write */
Expand Down
3 changes: 2 additions & 1 deletion src/lib/OpenEXRCore/internal_structs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ struct _priv_exr_context_t
#endif
uint8_t disable_chunk_reconstruct;
uint8_t legacy_header;
uint8_t _pad[6];
uint8_t _pad[2];
uint32_t orig_version_and_flags;
};

#define EXR_CONST_CAST(t, v) ((t) (uintptr_t) v)
Expand Down
58 changes: 35 additions & 23 deletions src/lib/OpenEXRCore/parse_header.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1596,13 +1596,25 @@ check_populate_type (
}

if (strcmp ((const char*) outstr, "scanlineimage") == 0)
curpart->storage_mode = EXR_STORAGE_SCANLINE;
{
if (ctxt->has_nonimage_data || ctxt->is_multipart)
curpart->storage_mode = EXR_STORAGE_SCANLINE;
}
else if (strcmp ((const char*) outstr, "tiledimage") == 0)
curpart->storage_mode = EXR_STORAGE_TILED;
{
if (ctxt->has_nonimage_data || ctxt->is_multipart)
curpart->storage_mode = EXR_STORAGE_TILED;
}
else if (strcmp ((const char*) outstr, "deepscanline") == 0)
curpart->storage_mode = EXR_STORAGE_DEEP_SCANLINE;
{
if (ctxt->has_nonimage_data || ctxt->is_multipart)
curpart->storage_mode = EXR_STORAGE_DEEP_SCANLINE;
}
else if (strcmp ((const char*) outstr, "deeptile") == 0)
curpart->storage_mode = EXR_STORAGE_DEEP_TILED;
{
if (ctxt->has_nonimage_data || ctxt->is_multipart)
curpart->storage_mode = EXR_STORAGE_DEEP_TILED;
}
else
{
rv = ctxt->print_error (
Expand Down Expand Up @@ -2478,6 +2490,7 @@ read_magic_and_flags (exr_context_t ctxt, uint32_t* outflags, uint64_t* initpos)

flags = magic_and_version[1];

ctxt->orig_version_and_flags = flags;
ctxt->version = flags & EXR_FILE_VERSION_MASK;
if (ctxt->version != 2)
{
Expand Down Expand Up @@ -2571,27 +2584,26 @@ internal_exr_parse_header (exr_context_t ctxt)
{
if (ctxt->has_nonimage_data || ctxt->is_multipart)
{
if (ctxt->strict_header)
{
rv = ctxt->print_error (
ctxt,
EXR_ERR_FILE_BAD_HEADER,
"Invalid combination of version flags: single part found, but also marked as deep (%d) or multipart (%d)",
(int) ctxt->has_nonimage_data,
(int) ctxt->is_multipart);
priv_destroy_scratch (&scratch);
return internal_exr_context_restore_handlers (ctxt, rv);
}
else
{
// assume multipart for now
ctxt->is_singlepart_tiled = 0;
}
// this appears to always be fatal, so do not check strict / not
rv = ctxt->print_error (
ctxt,
EXR_ERR_FILE_BAD_HEADER,
"Invalid combination of version flags: single part flag found, but also marked as deep (%d) or multipart (%d)",
(int) ctxt->has_nonimage_data,
(int) ctxt->is_multipart);
priv_destroy_scratch (&scratch);
return internal_exr_context_restore_handlers (ctxt, rv);
}
curpart->storage_mode = EXR_STORAGE_TILED;
}
else
curpart->storage_mode = EXR_STORAGE_SCANLINE;

/* leave storage mode uninitialized until we encounter the type */
if (!ctxt->has_nonimage_data && !ctxt->is_multipart)
{
if (ctxt->is_singlepart_tiled)
curpart->storage_mode = EXR_STORAGE_TILED;
else
curpart->storage_mode = EXR_STORAGE_SCANLINE;
}

do
{
Expand Down
79 changes: 60 additions & 19 deletions src/lib/OpenEXRCore/validation.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <limits.h>
#include <math.h>
#include <stdio.h>
#include <string.h>

/**************************************/

Expand Down Expand Up @@ -407,37 +408,77 @@ validate_part_type (exr_context_t f, exr_priv_part_t curpart)
// TODO: there are probably more tests to add here...
if (curpart->type)
{
int rv;
const char *expectedtype = NULL;
exr_result_t rv;

// see if the type overwrote the storage mode
if (f->is_singlepart_tiled &&
curpart->storage_mode != EXR_STORAGE_TILED)
if (f->is_singlepart_tiled)
{
// mismatch between type attr and file flag. c++ believed the
// flag first and foremost
curpart->storage_mode = EXR_STORAGE_TILED;

// TODO: define how strict we should be
//exr_attr_list_remove( f, &(curpart->attributes), curpart->type );
//curpart->type = NULL;
f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"attribute 'type': Mismatch between file flags and type string '%s', believing file flags",
curpart->type->string->str);
if (f->is_multipart || f->num_parts > 1)
return f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"Multipart files cannot have the tiled bit set");

if (curpart->storage_mode != EXR_STORAGE_TILED)
{
curpart->storage_mode = EXR_STORAGE_TILED;

if (f->strict_header)
{
return f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"attribute 'type': Single part tiled flag set but not marked as tiled storage type");
}
}
}

if (curpart->storage_mode == EXR_STORAGE_SCANLINE)
expectedtype = "scanlineimage";
else if (curpart->storage_mode == EXR_STORAGE_TILED)
expectedtype = "tiledimage";
else if (curpart->storage_mode == EXR_STORAGE_DEEP_SCANLINE)
expectedtype = "deepscanline";
else if (curpart->storage_mode == EXR_STORAGE_DEEP_TILED)
expectedtype = "deeptile";

if (expectedtype && 0 != strcmp (curpart->type->string->str, expectedtype))
{
if (f->mode == EXR_CONTEXT_WRITE) return EXR_ERR_INVALID_ATTR;

rv = exr_attr_string_set_with_length (
(exr_context_t) f, curpart->type->string, "tiledimage", 10);
if (rv != EXR_ERR_SUCCESS)
if (f->strict_header)
{
return f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"attribute 'type': Mismatch between file flags and type attribute, unable to fix");
"attribute 'type': Type should be '%s' but set to '%s', believing file flags",
expectedtype,
curpart->type->string->str);
}
else
{
/* C++ silently changed this */
rv = exr_attr_string_set (
f, curpart->type->string, expectedtype);

if (rv != EXR_ERR_SUCCESS)
return f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"attribute 'type': Mismatch between file flags and type attribute, unable to fix");
}
}
}

if (curpart->storage_mode == EXR_STORAGE_LAST_TYPE)
{
return f->print_error (
f,
EXR_ERR_INVALID_ATTR,
"Unable to determine data storage type for part");
}

return EXR_ERR_SUCCESS;
}

Expand Down
68 changes: 62 additions & 6 deletions src/lib/OpenEXRCore/write_header.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,67 @@ save_attr (exr_context_t ctxt, const exr_attribute_t* a)
return rv;
}

/**************************************/

exr_result_t internal_exr_calc_header_version_flags (exr_const_context_t ctxt, uint32_t *flags)
{
*flags = 2; // EXR_VERSION

if (ctxt->is_multipart) *flags |= EXR_MULTI_PART_FLAG;

if (ctxt->max_name_length > EXR_SHORTNAME_MAXLEN)
{
int longnamefound = 0;

for ( int p = 0; p < ctxt->num_parts; ++p )
{
exr_priv_part_t curpart = ctxt->parts[p];
for ( int a = 0; a < curpart->attributes.num_attributes; ++a )
{
exr_attribute_t *cura = curpart->attributes.entries[a];
if (cura->name_length > EXR_SHORTNAME_MAXLEN ||
cura->type_name_length > EXR_SHORTNAME_MAXLEN)
{
longnamefound = 1;
break;
}

// the original C++ side assumes there was
// only one channel list (who would have multiple)
// but let's not make that same assertion and check names
// if we encounter any channel list named anything
if (cura->type == EXR_ATTR_CHLIST)
{
const exr_attr_chlist_t* chlist = cura->chlist;
int nc = chlist->num_channels;

for ( int c = 0; c < nc; ++c )
{
const exr_attr_chlist_entry_t *ce = chlist->entries + c;
if (ce->name.length > EXR_SHORTNAME_MAXLEN)
{
longnamefound = 1;
break;
}
}
}
}

if (longnamefound)
break;
}

if (longnamefound)
*flags |= EXR_LONG_NAMES_FLAG;
}

if (ctxt->has_nonimage_data) *flags |= EXR_NON_IMAGE_FLAG;
if (ctxt->is_singlepart_tiled) *flags |= EXR_TILED_FLAG;

return EXR_ERR_SUCCESS;
}


/**************************************/

exr_result_t
Expand All @@ -606,12 +667,7 @@ internal_exr_write_header (exr_context_t ctxt)
uint32_t flags;
uint8_t next_byte;

flags = 2; // EXR_VERSION
if (ctxt->is_multipart) flags |= EXR_MULTI_PART_FLAG;
if (ctxt->max_name_length > EXR_SHORTNAME_MAXLEN)
flags |= EXR_LONG_NAMES_FLAG;
if (ctxt->has_nonimage_data) flags |= EXR_NON_IMAGE_FLAG;
if (ctxt->is_singlepart_tiled) flags |= EXR_TILED_FLAG;
rv = internal_exr_calc_header_version_flags (ctxt, &flags);

magic_and_version[0] = 20000630;
magic_and_version[1] = flags;
Expand Down

0 comments on commit e7f1099

Please sign in to comment.