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.

Sign up for GitHub

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

Jump to bottom

Fixed Improper Method Call: Replaced mktemp #33874

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fixed Improper Method Call: Replaced mktemp #33874

wants to merge 10 commits into from
+4 −2

Conversation

fazledyn-or
Copy link

Description

While triaging your project, our bug fixing tool generated the following message(s)-

In file: export_olx.py, there is a method that creates a temporary file using an unsafe API mktemp. The use of this method is discouraged in the Python documentation. iCR suggested that a temporary file should be created using mkstemp which is a safe API. iCR replaced the usage of mktemp with mkstemp.

Resources Related to mktemp

Changes

  • Replaced mktemp() method with mkstemp()

Previously Found & Fixed

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).
- Git Commit SignOff documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 4, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 4, 2023 •
edited
Loading

Thanks for the pull request, @fazledyn-or!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@mphilbrick211
Copy link

Hi @fazledyn-or, thank you for this contribution! Please let me know if you have any questions regarding completing a CLA form. Thanks!

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 4, 2023
@fazledyn-or
Copy link
Author

Hi @fazledyn-or, thank you for this contribution! Please let me know if you have any questions regarding completing a CLA form. Thanks!

Yeah, thanks for asking. Actually, I do need some help.
image

What do I put in these two fields?

@e0d
Copy link
Contributor

e0d commented Dec 14, 2023

@fazledyn-or I notice there are some commit-lint failures. Please note that we use conventional commits across Open edX projects. You can read about the details here. Can you please amend your commit messages to follow our standard?

@fazledyn-or in contributions field you can just put "N/A."

Are you contributing on bahalf of OpenRefactory-Inc or on your own behalf?

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 14, 2023
@fazledyn-or fazledyn-or force-pushed the Fix_Improper_Method_Call_mktemp branch from 0d4f25d to cf64849 Compare December 18, 2023 06:59
@fazledyn-or fazledyn-or force-pushed the Fix_Improper_Method_Call_mktemp branch from cf64849 to 560dc44 Compare December 18, 2023 07:14
@fazledyn-or
Copy link
Author

@fazledyn-or I notice there are some commit-lint failures. Please note that we use conventional commits across Open edX projects. You can read about the details here. Can you please amend your commit messages to follow our standard?

I've updated my commit message. Please have a look

@mphilbrick211
Copy link

Hi @fazledyn-or! Just checking to see if you plan to pursue this pull request? If so, it looks like you'll need to re-run the checks. Thanks!

@fazledyn-or
Copy link
Author

Hi @fazledyn-or! Just checking to see if you plan to pursue this pull request? If so, it looks like you'll need to re-run the checks. Thanks!

Yes, I do. My first commit message had a lint failure, so I updated the commit message and pushed. Looks like the workflows need to be re-run by one of the maintainers. Without re-running them, I can't say whether I need to update something or not.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 22, 2024
@mphilbrick211
Copy link

Hi @fazledyn-or! Just checking to see if you plan to pursue this pull request? If so, it looks like you'll need to re-run the checks. Thanks!

Yes, I do. My first commit message had a lint failure, so I updated the commit message and pushed. Looks like the workflows need to be re-run by one of the maintainers. Without re-running them, I can't say whether I need to update something or not.

@fazledyn-or great! I'll mark this for our team to re-run the checks.

@e0d
Copy link
Contributor

e0d commented Feb 26, 2024

Your branch is behind the base. I've pulled in changes from master as a merge commit which will update your branch and cause the tests to be re-run.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 26, 2024
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 19, 2024
@e0d
Copy link
Contributor

e0d commented Mar 22, 2024

Your branch is behind the base. I've pulled in changes from master as a merge commit which will update your branch and cause the tests to be re-run.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 22, 2024
@mariajgrimaldi mariajgrimaldi self-assigned this May 6, 2024
@mphilbrick211 mphilbrick211 added the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label May 16, 2024
@mphilbrick211
Copy link

Your branch is behind the base. I've pulled in changes from master as a merge commit which will update your branch and cause the tests to be re-run.

Hi @fazledyn-or! Is this still in-progress?

@mphilbrick211 mphilbrick211 removed the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Jul 30, 2024
@fazledyn-or
Copy link
Author

fazledyn-or commented Jul 30, 2024 via email

@mphilbrick211
Copy link

Yes. I don't think I have anything more to add. I've already made changes that were required, filled and submitted the form. If you need me to do anything to get it merged to the source code, please let me know. I'll be glad to do so.
…

@fazledyn-or - It looks like a few more tests need to be run. Please let me know if you have any questions!

@fazledyn-or
Copy link
Author

fazledyn-or commented Jul 31, 2024 via email

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 23, 2024
@mphilbrick211 mphilbrick211 added needs reviewer assigned PR needs to be (re-)assigned a new reviewer and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Sep 9, 2024
@farhaanbukhsh farhaanbukhsh self-requested a review December 9, 2024 18:11
@mphilbrick211 mphilbrick211 removed the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Dec 10, 2024
farhaanbukhsh
farhaanbukhsh requested changes Jan 13, 2025
View reviewed changes
Copy link
Member

@farhaanbukhsh farhaanbukhsh left a comment

Choose a reason for hiding this comment

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

I have requested a change since mkstemp has a bug with file descriptors.

cms/djangoapps/contentstore/management/commands/export_olx.py
@@ -55,7 +55,8 @@ def handle(self, *args, **options):
pipe_results = False

if filename is None:
filename = mktemp()
fd, filename = mkstemp()
os.close(fd)
Copy link
Member

Choose a reason for hiding this comment

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

@fazledyn-or in addition to this we need to add os.unlink(filename), this is one of the bugs pointed out in python/cpython#86996.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farhaanbukhsh farhaanbukhsh Awaiting requested review from farhaanbukhsh

Requested changes must be addressed to merge this pull request.

Assignees

@mariajgrimaldi mariajgrimaldi

Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Status: Ready for Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fazledyn-or @openedx-webhooks @mphilbrick211 @e0d @farhaanbukhsh @mariajgrimaldi