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

Example fuzzer for RT-N12 B1 httpd #1480

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

libumem
Copy link

@libumem libumem commented Jul 8, 2024

Checklist

Which kind of PR do you create?

  • This PR only contains minor fixes.
  • This PR contains major feature update.
  • This PR introduces a new function/api for Qiling Framework.

Coding convention?

  • The new code conforms to Qiling Framework naming convention.
  • The imports are arranged properly.
  • Essential comments are added.
  • The reference of the new code is pointed out.

Extra tests?

  • No extra tests are needed for this PR.
  • I have added enough tests for this PR.
  • Tests will be added after some discussion and review.

Changelog?

  • This PR doesn't need to update Changelog.
  • Changelog will be updated after some proper review.
  • Changelog has been updated in my PR.

Target branch?

  • The target branch is dev branch.

One last thing


Copy link
Member

@elicn elicn left a comment

Choose a reason for hiding this comment

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

Hi there and thanks for the contribution; more examples help people to moderate their learning curve.
For that very reason it is imperative that the examples will be as clean as possible and at the same time heavily commented. What are you doing and why you are doing things they way you do is key for other people to learn from the example. If too many things remain mysterious or unclear people will not learn too much, or worse, it even might confuse them.

Will appreciate you taking the time to go over the requested changes and make the necessary adjustments. Also here are a few general comments:

  1. Try to adhere to PEP8 conventions, it really helps to keep the code clean and tidy
  2. Use proper annotations also for return values, not only for arguments
  3. Add more comments explaining what you are doing; things that are obvious to you might not be for others
  4. I am not sure we are allowed to attach the firmware files here. If you have a direct like to the firmware on the vendor's website, that would be great

Cheers!

examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
examples/fuzzing/rt_n12/fuzz.py Outdated Show resolved Hide resolved
@libumem libumem changed the title Example fuzzer for RT-N12 httpd Example fuzzer for RT-N12 B1 httpd Jul 19, 2024
@libumem
Copy link
Author

libumem commented Jul 19, 2024

Try to adhere to PEP8 conventions, it really helps to keep the code clean and tidy

I've since run a linter to address this.

Use proper annotations also for return values, not only for arguments

Should be fixed now.

Add more comments explaining what you are doing; things that are obvious to you might not be for others

Added multiple comment sections to aid understanding.

I am not sure we are allowed to attach the firmware files here. If you have a direct like to the firmware on the vendor's website, that would be great

Removed firmware file (which was the wrong one anyway) and added a link to the manufacturer's page.

@elicn
Copy link
Member

elicn commented Jul 21, 2024

Great, thanks for making the changes.
Please see the two remaining comments.

(Make sure to tag me, so I will get notified)

@libumem
Copy link
Author

libumem commented Jul 21, 2024

@elicn Addressed the feedback you gave. Much appreciated.

@libumem
Copy link
Author

libumem commented Aug 23, 2024

Will someone else please approve this?

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.

3 participants