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] Fix segfault in add_cc_sub_text and initialize to NULL in init_encoder #950

Merged
merged 1 commit into from
Mar 5, 2018

Conversation

saurabhshah0410
Copy link
Contributor

@saurabhshah0410 saurabhshah0410 commented Mar 3, 2018

Please prefix your pull request with one of the following: [FEATURE] [FIX] [IMPROVEMENT].

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

This commit adds some checks to avoid segmentation faults.

  • In add_cc_sub_text(), strdup will cause a segfault if we duplicate an
    empty string.

  • In init_encoder(), initialize pointer fields to NULL to avoid random
    addressing so we can avoid illegal memory accessing and segfaults in
    other places.

@@ -924,7 +924,8 @@ void dinit_encoder(struct encoder_ctx **arg, LLONG current_fts)
write_subtitle_file_footer(ctx, ctx->out + i);
}

free_encoder_context(ctx->prev);
if(!ccx_options.hardsubx) // Don't free ctx->prev in case of hardsubx to avoid segfault
free_encoder_context(ctx->prev);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that call to free_encoder_context segfaulting? That's what we should fix, not just not do the call because it segfaults... is it because ctx->prev is null, because it ends in a double-free, or what?

This seems like hiding the problem, not fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cfsmp3 Yes, you are right. I didn't go deep enough to investigate the cause of the segfault before your comment.

So the prev field of an encoder_ctx is used to process dvb subtitles. They are not at all used in the code that involves processing hard subtitles. And when an encoder_ctx is initialized in init_encoder function, the prev field is not set to NULL and hence, it gets some random value(address). So, ctx->prev != NULL and free_encoder_context tries to free the fields of ctx->prev, which do not exist, and hence a segfault. So this fix is wrong and the actual fix is very simple. Please check my latest fix.

This commit adds some checks to avoid segmentation faults.

* In `add_cc_sub_text()`, strdup will cause a segfault if we duplicate an
  empty string.

* In `init_encoder()`, initialize pointer fields to NULL to avoid random
  addressing so we can avoid illegal memory accessing and segfaults in
  other places.
@saurabhshah0410 saurabhshah0410 changed the title [FIX] Avoid segmentation faults in dinit_encoder() and add_cc_sub_text() [FIX] Fix segfault in add_cc_sub_text and initialize to NULL in init_encoder Mar 5, 2018
@cfsmp3 cfsmp3 merged commit f46e3dc into CCExtractor:master Mar 5, 2018
@saurabhshah0410 saurabhshah0410 deleted the bugfix branch March 6, 2018 13:06
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.

2 participants