search

wingman-jr-addon / wingman_jr

This is the official repository (https://github.com/wingman-jr-addon/wingman_jr) for the Wingman Jr. Firefox addon, which filters NSFW images in the browser fully client-side: https://addons.mozilla.org/en-US/firefox/addon/wingman-jr-filter/ Optional DNS-blocking using Cloudflare's 1.1.1.1 for families! Also, check out the blog!
https://wingman-jr.blogspot.com/
Other
35 stars 6 forks source link

EBML Cluster Finder Loop #167

Closed s0try closed 2 years ago

s0try commented 2 years ago

Hello, thanks for working on such a neat project. I was familiarizing myself with the code and came upon this function to find the clusters in a WEBM file. https://github.com/wingman-jr-addon/wingman_jr/blob/0deb0a7b4fa32c7086a8949fafc343305c961961/ebml.js#L212-L223 Since this function is called with indexes greater than 0, it would seem that for all calls where index is greater than 0, u8Array[i] is being compared to undefined since i values larger than 3 will be out of bounds for the EBML_CLUSTER_ID_ARRAY array.

Fixing this function is trivial, but I am unsure how fixing it would affect the program as a whole, so I just wanted to bring it to your attention.

wingman-jr-addon commented 2 years ago

Yep, you're definitely right that's a bug. This branch of code only affects how the YouTube scanning of WebM works. Fortunately the impact is (I think) relatively low: the vast majority of videos I've browsed on YouTube seem to serve up as MP4, and I believe in at least some cases we additionally got lucky because it is not uncommon for the cluster to actually be at 0. At any rate, thanks @s0try ! Fixed here: https://github.com/wingman-jr-addon/wingman_jr/commit/3b0ab301764df5e08505803b83203f270d1826a7

s0try commented 2 years ago

Of course, while I'm at it, I also wanted to ask about this section of the same file. https://github.com/wingman-jr-addon/wingman_jr/blob/3b0ab301764df5e08505803b83203f270d1826a7/ebml.js#L251-L263 An array is defined above the function, but then the array from the clustering section is used instead. Is this also a bug?

wingman-jr-addon commented 2 years ago

@s0try Yep, that one is too. Fixed here: https://github.com/wingman-jr-addon/wingman_jr/commit/6d09659d5ad640197db7bb0ee12eb9635155325b

Fortunately this one also has relatively low impact: it was only being used externally in vidDetectType, which would trigger when the MIME type detected was application/octet-stream but we needed to probe it further for detecting video. The end result is that we were always falling back to video/* rather than video/webm, which in turn downstream was probably doing the right thing.

Thanks for the check! These things are often a little tricky to trigger unless there is a clear endpoint that doesn't change with a known MIME type etc.

Find any other gotchas?

s0try commented 2 years ago

Nothing else for now, will let you know 👍