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

Fix ddos created by requesting infinitely for the same chunks #1423

Closed
wants to merge 1 commit into from

Conversation

302Gerben
Copy link

Description

As stated in the following discussion, videojs player requests in certain situations indefinitely for the same chunks which results in a ddos being fired at the server. This issue keeps getting ignored/closed automatically.

#1000 (comment)

This situation happens when there is a network congestion or just really slow internet speeds. (Chrome dev tools, throttling slow 3G for example) the player starts buffering after a few seconds. When the network recovers to normal speed, the player keeps requesting the same chunk over and over. (Imagine you have a hundreds of visitors..)

Specific Changes proposed

Check if there are seekable points.

Requirements Checklist

  • Bug fixed
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Aug 31, 2023

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@adrums86
Copy link
Contributor

adrums86 commented Sep 1, 2023

Thanks for submitting this PR and linking the issue! I read through everything and think I understand what the issue is, this fix may be preventing a problematic seek, but I'm not sure it's doing it in the way that's expected in the original issue. The seekable range is a TimeRanges object that will return a start and end, not correlating directly to the number of segments that are currently buffered. If the seekable ranges length is > 1 it essentially means there are 2 ranges that are seekable, for example if there's a buffer gap.
That said, I do think this needs to be addressed ASAP. I'm wondering if we can just remove the entire block of code that causes this issue? Per the TODO at the top of the function the scenario where a seek is before the seekable range or into a gap during live playback seems to be handled with the buffered checks below.

@amtins
Copy link
Contributor

amtins commented Sep 3, 2023

I may be totally off topic, as different comments in different issues seem to mention different problems.

@adrums86 I went through the different issues and it reminded me of videojs/mux.js#430 because of #1049 (comment).

I tried to make a fix at the mux level that is based on the same logic but in m2ts ElementaryStream amtins/mux.js#1.

It seems to fix the issue (but which one? 😅), the stream used to test is https://snapstream-dev-test-public.s3.us-east-1.amazonaws.com/videojs-pts-rollover-issue/playlist.m3u8

How to test

  • Open the page and start playback
  • Seek near the end (around 00:50:00)

Before
https://videojs-http-streaming.netlify.app/?debug=false&autoplay=false&muted=false&fluid=false&minified=false&sync-workers=false&liveui=false&llhls=true&url=https%3A%2F%2Fsnapstream-dev-test-public.s3.us-east-1.amazonaws.com%2Fvideojs-pts-rollover-issue%2Fplaylist.m3u8&type=application%2Fx-mpegURL&keysystems=&buffer-water=false&exact-manifest-timings=false&pixel-diff-selector=false&network-info=false&dts-offset=false&override-native=true&preload=auto&mirror-source=true&forced-subtitles=false

After
https://amtins.github.io/http-streaming-mux/

However, if we take the stream, which is unavailable, the player continually tries each playlist. This seems to be expected behavior, as the player waits for a child playlist to become available again.

@video-archivist-bot
Copy link

Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:

@adrums86
Copy link
Contributor

adrums86 commented Sep 6, 2023

Great catch @amtins ! Lets get that PR merged this week and update VHS with a new mux and see if this alleviates the issue for everyone.

@amtins
Copy link
Contributor

amtins commented Sep 6, 2023

@adrums86 I'll try to do a proper PR tomorrow, but maybe I could use a little help with the unit tests. However, it seems odd that I should have done this, as the various piplines call TimestampRolloverStream

@adrums86
Copy link
Contributor

This might be fixed in #1433 can anyone confirm?

@GeppieNL
Copy link

This might be fixed in #1433 can anyone confirm?

We can confirm it is fixed by 1433! 🎉

@adrums86
Copy link
Contributor

@GeppieNL Thanks for the confirmation. Closing this as it appears this is fixed in #1433.

@adrums86 adrums86 closed this Jan 16, 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.

7 participants