Hi On Tue, Aug 19, 2025 at 06:33:03PM +0300, [email protected] wrote: > The branch, master has been updated > via 4c3f94f2651ce4d6834fbef10e5c287f25720ac9 (commit) > via fe496b0308f1911138698b92bbafac582fa455d7 (commit) > via 535a07d14e16eb2930d2b1e16605eafa95959139 (commit) > via 8132ee046d1a03c5913759c50fce7beda8d3d627 (commit) > via 5d4f873ff31f86f9ae1927f37ca0fcb78da70512 (commit) > via 1e816ebefee0ef479fa76bd2a1fb9ec5d5e807df (commit) > via 93a8091015978c44462626409af71789080bbef4 (commit) > via 303f60684f25ce63951480ef0f759acf41aac938 (commit) > via 5caaadee7954f2014d9e0a17d1edfb961893bcf1 (commit) > via d3190a64c366a79091fe47fddf93c09a7d988802 (commit) > via 44af3829796427b89c4b76b56652cc5932ada616 (commit) > via f5ad1c910c168d05cb01315773ab0bd094c9372f (commit) > via e3aa1154aab2656c91ce61915f79516d9b563b61 (commit) > via c6cc2115f45ac26ae42442f8796996eb410f4028 (commit) > via 52dba25661305e3c4a6209d46aea43cd327c960e (commit) > via bfb17d26306592c85cf0c4e909099c621177b062 (commit) > via ba2ea285e0f270a0a885b414cafaace6a89b9a91 (commit) > via ad77345a5d14862f4701e5ad422b03b14934a5b9 (commit) <------- This > mail reviews this one > via bb90b262d6d23f1bca3587a48abc15b951cbbf05 (commit) < > via a99fff4e2d4058c57599ba0b968862af82bdad5b (commit) <- Reveiwed > previously > from a6b5a382dd7ecdd27c5d0ebba688e1db409d18fd (commit) >
> commit ad77345a5d14862f4701e5ad422b03b14934a5b9 > Author: Leo Izen <[email protected]> > AuthorDate: Thu Mar 6 17:53:41 2025 -0500 > Commit: Leo Izen <[email protected]> > CommitDate: Tue Aug 19 11:26:46 2025 -0400 > > avcodec/exif: add EXIF parser and struct API > > This commit adds a structure to contain parsed EXIF metadata, as well > as code to read and write that struct from/to binary EXIF buffers. Some > internal functions have been moved to exif_internal.h. Code to read > from this new struct and write to an AVDictionary **dict has been added > as well in order to preserve interoperability with existing callers. The metadata format is not preserved, the commit message is not correct > The only codec changes so far as of this commit are to call these > interop functions, but in future commits there will be codec changes to > use the new parsing routines instead. > > Signed-off-by: Leo Izen <[email protected]> > [...] > -static const char *exif_get_tag_name(uint16_t id) > -{ > - int i; > +/* same as type_sizes but with string == 1 */ > +static const size_t exif_sizes[] = { > + [0] = 0, > + [AV_TIFF_BYTE] = 1, > + [AV_TIFF_STRING] = 1, > + [AV_TIFF_SHORT] = 2, > + [AV_TIFF_LONG] = 4, > + [AV_TIFF_RATIONAL] = 8, > + [AV_TIFF_SBYTE] = 1, > + [AV_TIFF_UNDEFINED] = 1, > + [AV_TIFF_SSHORT] = 2, > + [AV_TIFF_SLONG] = 4, > + [AV_TIFF_SRATIONAL] = 8, > + [AV_TIFF_FLOAT] = 4, > + [AV_TIFF_DOUBLE] = 8, > + [AV_TIFF_IFD] = 4, > +}; > > - for (i = 0; i < FF_ARRAY_ELEMS(tag_list); i++) { > +const char *av_exif_get_tag_name(uint16_t id) > +{ > + for (size_t i = 0; i < FF_ARRAY_ELEMS(tag_list); i++) { > if (tag_list[i].id == id) > return tag_list[i].name; > } This design assumes ids are globally unique, but they are not. heres an example: InteroperabilityIndex uses 1 GPSLatitudeRef uses 1 too [...] > +static int exif_decode_tag(void *logctx, GetByteContext *gb, int le, > + int depth, AVExifEntry *entry) > +{ > + int ret = 0, makernote_offset = -1, tell, is_ifd, count; > + enum AVTiffDataType type; > + uint32_t payload; > + > + /* safety check to prevent infinite recursion on malicious IFDs */ > + if (depth > 3) > + return AVERROR_INVALIDDATA; > + > + tell = bytestream2_tell(gb); > + > + entry->id = ff_tget_short(gb, le); > + type = ff_tget_short(gb, le); > + count = ff_tget_long(gb, le); > + payload = ff_tget_long(gb, le); > > - if (!name) { > - name = buf; > - snprintf(buf, sizeof(buf), "0x%04X", id); > + av_log(logctx, AV_LOG_DEBUG, "TIFF Tag: id: 0x%04x, type: %d, count: %u, > offset: %d, " > + "payload: %" PRIu32 "\n", entry->id, type, > count, tell, payload); > + > + is_ifd = type == AV_TIFF_IFD || ff_tis_ifd(entry->id) || entry->id == > MAKERNOTE_TAG; > + > + if (is_ifd) { > + if (!payload) > + goto end; > + bytestream2_seek(gb, payload, SEEK_SET); > + } > + > + if (entry->id == MAKERNOTE_TAG) { > + makernote_offset = exif_get_makernote_offset(gb); > + if (makernote_offset < 0) > + is_ifd = 0; > + } > + > + if (is_ifd) { > + entry->type = AV_TIFF_IFD; > + entry->count = 1; > + entry->ifd_offset = makernote_offset > 0 ? makernote_offset : 0; > + if (entry->ifd_offset) { > + entry->ifd_lead = av_malloc(entry->ifd_offset); > + if (!entry->ifd_lead) > + return AVERROR(ENOMEM); > + bytestream2_get_buffer(gb, entry->ifd_lead, entry->ifd_offset); > + } > + ret = exif_parse_ifd_list(logctx, gb, le, depth + 1, > &entry->value.ifd); > + if (ret < 0 && entry->id == MAKERNOTE_TAG) { > + /* > + * we guessed that MakerNote was an IFD > + * but we were probably incorrect at this > + * point so we try again as a binary blob > + */ > + av_exif_free(&entry->value.ifd); > + av_log(logctx, AV_LOG_DEBUG, "unrecognized MakerNote IFD, > retrying as blob\n"); > + is_ifd = 0; > } > + } > > - ret = exif_add_metadata(logctx, count, type, name, NULL, > - gbytes, le, metadata); > + /* inverted condition instead of else so we can fall through from above > */ > + if (!is_ifd) { > + entry->type = type == AV_TIFF_IFD ? AV_TIFF_UNDEFINED : type; > + entry->count = count; > + bytestream2_seek(gb, count * exif_sizes[type] > 4 ? payload : tell + > 8, SEEK_SET); using unverified type as index into array, fixed in: commit 647138334abd6ea001a16a768eb12cc4156db5f9 Author: Michael Niedermayer <[email protected]> Date: Thu Sep 18 02:25:32 2025 +0200 avcodec/exif: check count in exif_decode_tag() Fixes: out of array access Fixes: integer overflow Fixes: poc_heap_bof Found-by: *2ourc3 (Salim LARGO) > + ret = exif_read_values(logctx, gb, le, entry); > } > > - bytestream2_seek(gbytes, cur_pos, SEEK_SET); > +end: > + bytestream2_seek(gb, tell + BASE_TAG_SIZE, SEEK_SET); > > return ret; > } > > - > -int ff_exif_decode_ifd(void *logctx, GetByteContext *gbytes, > - int le, int depth, AVDictionary **metadata) > +static int exif_parse_ifd_list(void *logctx, GetByteContext *gb, int le, > + int depth, AVExifMetadata *ifd) > { > - int i, ret; > - int entries; > + uint32_t entries; > + size_t required_size; > + void *temp; > > - entries = ff_tget_short(gbytes, le); > + av_log(logctx, AV_LOG_DEBUG, "parsing IFD list at offset: %d\n", > bytestream2_tell(gb)); > > - if (bytestream2_get_bytes_left(gbytes) < entries * 12) { > + if (bytestream2_get_bytes_left(gb) < 2) { > + av_log(logctx, AV_LOG_ERROR, "not enough bytes remaining in EXIF > buffer: 2 required\n"); > return AVERROR_INVALIDDATA; > } > > - for (i = 0; i < entries; i++) { > - if ((ret = exif_decode_tag(logctx, gbytes, le, depth, metadata)) < > 0) { > + entries = ff_tget_short(gb, le); > + if (bytestream2_get_bytes_left(gb) < entries * BASE_TAG_SIZE) { > + av_log(logctx, AV_LOG_ERROR, "not enough bytes remaining in EXIF > buffer. entries: %" PRIu32 "\n", entries); > + return AVERROR_INVALIDDATA; > + } > + > + ifd->count = entries; > + av_log(logctx, AV_LOG_DEBUG, "entry count for IFD: %u\n", ifd->count); > + > + if (av_size_mult(ifd->count, sizeof(*ifd->entries), &required_size) < 0) > + return AVERROR(ENOMEM); > + temp = av_fast_realloc(ifd->entries, &ifd->size, required_size); > + if (!temp) { > + av_freep(&ifd->entries); > + return AVERROR(ENOMEM); > + } > + ifd->entries = temp; > + > + /* entries have pointers in them which can cause issues if */ > + /* they are freed or realloc'd when garbage */ > + memset(ifd->entries, 0, required_size); > + > + for (uint32_t i = 0; i < entries; i++) { > + int ret = exif_decode_tag(logctx, gb, le, depth, &ifd->entries[i]); > + if (ret < 0) > return ret; > + } > + > + /* > + * at the end of an IFD is an pointer to the next IFD > + * or zero if there are no more IFDs, which is usually the case > + */ > + return ff_tget_long(gb, le); the offset here is not checked, just blindly returned, the calling code also doesnt check it. [...] > +static int exif_write_ifd(void *logctx, PutByteContext *pb, int le, const > AVExifMetadata *ifd) > +{ > + int offset, ret, tell; > + tell = bytestream2_get_bytes_left_p(pb); this is wrong, its mixing up offset with bytes left this was fixed in f5ad1c910c168d05cb01315773ab0bd094c9372f both where in the same pull request, a single pull request must not add a bug in one commit thats fixed in a subaequent one in the same PR [...] > +static int exif_attach_ifd(void *logctx, AVFrame *frame, const > AVExifMetadata *ifd, AVBufferRef *og) > +{ > + const AVExifEntry *orient = NULL; > + AVFrameSideData *sd; > + AVExifMetadata *cloned = NULL; > + AVBufferRef *written = NULL; > + int ret; > + > + for (size_t i = 0; i < ifd->count; i++) { > + const AVExifEntry *entry = &ifd->entries[i]; > + if (entry->id == ORIENTATION_TAG && entry->count > 0 && entry->type > == AV_TIFF_SHORT) { > + orient = entry; > + break; > + } > + } > + > + if (orient && orient->value.uint[0] > 1) { > + av_log(logctx, AV_LOG_DEBUG, "found nontrivial EXIF orientation: %" > PRIu64 "\n", orient->value.uint[0]); > + ret = attach_displaymatrix(logctx, frame, orient->value.uint[0]); > + if (ret < 0) { > + av_log(logctx, AV_LOG_WARNING, "unable to attach displaymatrix > from EXIF\n"); > + } else { > + const AVExifEntry *cloned_orient; > + cloned = av_exif_clone_ifd(ifd); > + if (!cloned) { > + ret = AVERROR(ENOMEM); > + goto end; > + } > + // will have the same offset in the clone as in the original > + cloned_orient = &cloned->entries[orient - ifd->entries]; > + cloned_orient->value.uint[0] = 1; > + } > + } > + > + ret = av_exif_ifd_to_dict(logctx, cloned ? cloned : ifd, > &frame->metadata); > + if (ret < 0) > + return ret; > + > + if (cloned || !og) { > + ret = av_exif_write(logctx, cloned ? cloned : ifd, &written, > AV_EXIF_TIFF_HEADER); > + if (ret < 0) > + goto end; > + } > + > + sd = av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_EXIF, written > ? written : og); > + if (!sd) { > + if (written) > + av_buffer_unref(&written); > + ret = AVERROR(ENOMEM); > + goto end; > + } > + > + ret = 0; > + > +end: > + if (og && written && ret >= 0) > + av_buffer_unref(&og); // as though we called new_side_data on og; > + av_exif_free(cloned); > + av_free(cloned); > + return ret; > +} this contains a leak [...] > +int av_exif_set_entry(void *logctx, AVExifMetadata *ifd, uint16_t id, enum > AVTiffDataType type, > + uint32_t count, const uint8_t *ifd_lead, uint32_t ifd_offset, const void > *value) > +{ > + void *temp; > + int ret = 0; > + AVExifEntry *entry = NULL; > + AVExifEntry src = { 0 }; > + > + if (!ifd || ifd->entries && !ifd->count || ifd->count && !ifd->entries > + || ifd_lead && !ifd_offset || !ifd_lead && ifd_offset > + || !value || ifd->count == 0xFFFFu) > + return AVERROR(EINVAL); > + > + for (size_t i = 0; i < ifd->count; i++) { > + if (ifd->entries[i].id == id) { > + entry = &ifd->entries[i]; > + break; > + } > + } > + > + if (entry) { > + exif_free_entry(entry); > + } else { > + size_t required_size; > + ret = av_size_mult(ifd->count + 1, sizeof(*ifd->entries), > &required_size); > + if (ret < 0) > + return AVERROR(ENOMEM); > + temp = av_fast_realloc(ifd->entries, &ifd->size, required_size); > + if (!temp) > + return AVERROR(ENOMEM); > + ifd->entries = temp; > + entry = &ifd->entries[ifd->count++]; > + } > + > + src.count = count; > + src.id = id; > + src.type = type; > + src.ifd_lead = (uint8_t *) ifd_lead; > + src.ifd_offset = ifd_offset; > + if (type == AV_TIFF_IFD) > + src.value.ifd = * (const AVExifMetadata *) value; > + else > + src.value.ptr = (void *) value; > + > + ret = exif_clone_entry(entry, &src); > + > + if (ret < 0) > + ifd->count--; This corrupts the state if a entry is changed that existed perviously and an error occurs [...] > +/** > + * Add an entry to the provided EXIF metadata struct. If one already exists > with the provided > + * ID, it will set the existing one to have the other information provided. > Otherwise, it > + * will allocate a new entry. > + * > + * This function reallocates ifd->entries using av_realloc and allocates > (using av_malloc) > + * a new value member of the entry, then copies the contents of value into > that buffer. This is untrue, the function uses av_fast_realloc() not av_realloc() > + */ > +int av_exif_set_entry(void *logctx, AVExifMetadata *ifd, uint16_t id, enum > AVTiffDataType type, > + uint32_t count, const uint8_t *ifd_lead, uint32_t > ifd_offset, const void *value); [...] > +TAG:ExifIFD/MakerNote/GPSAltitude=IMG:PowerShot SX200 IS JPEG > +TAG:ExifIFD/MakerNote/GPSTimeStamp=Firmware Version 1.00 this is clearly not correct, the parser is not identifying things correctly [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
