Skip to content

Fix for issue #108 - Make sure the buffer is large enough before ever… #110

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

Merged
merged 3 commits into from
Apr 19, 2023
Merged

Fix for issue #108 - Make sure the buffer is large enough before ever… #110

merged 3 commits into from
Apr 19, 2023

Conversation

dgnuff
Copy link
Contributor

@dgnuff dgnuff commented Apr 10, 2023

Added a const value for the buffer size.
Used that value for both the initial buffer allocation, and the buffer re-initialization in preparation for sending packets.

This prevents a buffer overflow when building the DHCPREQUEST packet because the buffer appears to have shrunk while processing the DHCPOFFER reply.

@tekktrik tekktrik requested a review from a team April 11, 2023 16:54
@tekktrik tekktrik linked an issue Apr 11, 2023 that may be closed by this pull request
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

The resizing of the global _BUFF takes place at line 270. I don't even see a reason for a global _BUFF (do you). Could you keep your _BUFF_SIZE = const(318) change, and remove the global _BUFF at line 90? Then also remove the global _BUFF declaration at line 268. That will make the code use local buffers in both cases. Then if you could test, that woud be great.

If that works, then _BUFF could be renamed everywhere to buff or buf. Since it's not a global anymore, there's no reason to give it an underscore, all-caps name.

@dgnuff
Copy link
Contributor Author

dgnuff commented Apr 14, 2023

Running my eyes over it, I can see no reason at all for _BUFF to be global. And indeed, this is a copy-book example of why globals are bad, because code in function A modifies the global, causing a bug in function B.

@dgnuff dgnuff requested a review from dhalbert April 19, 2023 17:46
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Great, thanks for fixing this and removing the unnecessary global!

@dhalbert dhalbert merged commit b3e0c7e into adafruit:main Apr 19, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 20, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 2.4.1 from 2.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#107 from BiffoBear/raw_dns
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#110 from dgnuff/dhcp-buffer-overflow
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#106 from BiffoBear/bugfix_socket_send

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 2.5.0 from 2.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#48 from Neradoc/respond-to-file-head
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#47 from Neradoc/allow-no-route

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
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.

DHCP has a buffer overrun when transmitting the DHCPREQUEST packet
2 participants