Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to fix buffer overflows with rDNS tags #63

Closed
wants to merge 4 commits into from

Conversation

ocococococ
Copy link

@ocococococ ocococococ commented Jan 13, 2023

As asked,
I created this PR with a test_reverseDNS.sh script to test adding many rDNS tags to already provided issue-32.mp4.

Without this patch, there are still buffer overflows.
Corresponding ASAN report is already uploaded here: #62

@ocococococ
Copy link
Author

ocococococ commented Jan 16, 2023

FYI, as I am mostly a Mac user, and with latest commit, binary output of rDNS tags appears identical to
those generated by tools like Subler or Musicbrainz Picard.
Moreover, It also seems ok with freyr-js regarding these comments before replacing AtomicParsley by lofty-rs.
Finally, I managed to check that even under Windows, it seems to work too.

Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks out! Thanks! 💪🏽

// https://github.com/wez/atomicparsley/issues/44
parsedAtoms[rDNS_name_atom].AtomicLength = 12;
parsedAtoms[rDNS_name_atom].AtomicLength = 16;
Copy link
Contributor

@howyallare howyallare Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was right the first time being initialised to 12. Otherwise you can get a buffer overrun and an invalid "name" atom written to file when the length of its value is 16.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried with value 12 instead of 16 on a single file and indeed it seems to work.

@ghjimmy
Copy link

ghjimmy commented Jun 8, 2023

Without this patch AtomicParsley on Apple Silicon native corrupts files when using --rDNSatom

% sh redo
Before and after tagging with /tmp/AtomicParsley.brew
197632 -rw-r--r--@ 1 jimmy  staff  93658182  8 Jun 09:37 movie.mp4
4196048 -rw-r--r--@ 1 jimmy  staff  2148376318  8 Jun 09:37 movie.mp4
Before and after tagging with /tmp/AtomicParsley.arm
182928 -rw-r--r--@ 1 jimmy  staff  93658182  8 Jun 09:37 movie.mp4
182664 -rw-r--r--@ 1 jimmy  staff  93521292  8 Jun 09:37 movie.mp4
% cat redo
AP1=/tmp/AtomicParsley.brew
AP2=/tmp/AtomicParsley.arm
f=movie.mp4
for AP in "$AP1" "$AP2"
do
    cp /tmp/$f .
    echo Before and after tagging with $AP
    ls -sl $f
    $AP "$f" --overWrite --metaEnema --artwork REMOVE_ALL  \
--rDNSatom "<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC '-//Apple//DTD PLIST 1.0//EN' 'http://www.apple.com/DTDs/PropertyList-1.0.dtd'>
<plist version='1.0'>
<dict>
	<key>cast</key>
	<array>
		<dict>
			<key>name</key>
			<string>Peter Pepper</string>
		</dict>
	</array>
</dict>
</plist>" name="iTun" domain="com.apple.iTunes" \
--stik 'Movie' \
--title 'Diamond' \
--year '2010-03-18-T12:00:00Z' --freefree >/dev/null 2>&1
    ls -sl $f
done

@wez
Copy link
Owner

wez commented Jun 8, 2024

Resolved by #71

@wez wez closed this Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants