Skip to content

fix: Bounds-check read_from_file restores with snprintf - #64

Open
VedantMadane wants to merge 1 commit into
RomanAlexandroff:mainfrom
VedantMadane:fix/issue-34
Open

VedantMadane wants to merge 1 commit into
RomanAlexandroff:mainfrom
VedantMadane:fix/issue-34

Conversation

@VedantMadane

Copy link
Copy Markdown

Summary

Bounds-check read_from_file restores with snprintf

Changes

  • file_system.cpp: replace strcpy with bounds-checked snprintf (4)

Fixes #34

@RomanAlexandroff RomanAlexandroff left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hi and welcome!

In secret_verification() and data_integrity_check(), snprintf and sizeof will work just fine.

However, in read_from_file(), sizeof will not work as you expect because here 'output' is a pointer to the char array, not the array itself. Calling sizeof(output) will return the size of the pointer which on a 32-bit system is just 4 bytes, causing snprintf to always copy at most 3 characters + a nul-terminator.

Instead, you've got to calculate the size to copy first and then pass it into read_from_file() e.g. like this:
read_from_file(const char* file_name, char* output, size_t output_size)

@VedantMadane

Copy link
Copy Markdown
Author

Thank you for the review and explanation! I have updated
ead_from_file() to accept size_t output_size, updated the header declaration, and passed the explicit destination buffer sizes (sizeof(rtc_g.Secret) and sizeof(rtc_g.chat_id)) from
estore_data_value().

read_from_file now takes an explicit size_t output_size so snprintf does
not use sizeof(pointer). Call sites pass sizeof(rtc_g.Secret) and
sizeof(rtc_g.chat_id) via restore_data_value.

Fixes RomanAlexandroff#34

Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
@VedantMadane

Copy link
Copy Markdown
Author

Ready for re-review.

Maintainer feedback addressed

  • read_from_file(const char* file_name, char* output, size_t output_size) now takes an explicit destination size (no sizeof on the pointer).
  • Header declaration updated to match.
  • Call sites pass real buffer sizes: sizeof(rtc_g.Secret) / sizeof(rtc_g.chat_id) via restore_data_value(..., destination_size).
  • snprintf(output, output_size, %s, ...) uses that size; empty output_size rejected at entry.

Cleanup

  • Removed leftover _pr_meta.json from the branch.
  • Squashed onto current main as a single DCO-signed commit (55b4d15).

Please take another look when convenient. Thank you!

@VedantMadane

Copy link
Copy Markdown
Author

Hi @RomanAlexandroff — gentle re-review bump.

read_from_file takes explicit output_size; leftover meta file removed. Ready when you are. Thanks!

@VedantMadane

Copy link
Copy Markdown
Author

Hi @RomanAlexandroff — gentle re-review bump when you have a moment. Thanks!

@VedantMadane

Copy link
Copy Markdown
Author

Verified: read_from_file(const char* file_name, char* output, size_t output_size) takes output_size and uses snprintf(output, output_size, "%s", buffer.c_str()) — no sizeof on the pointer. Call sites pass the destination buffer size.

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.

[Issue] Missing bounds checking in read_from_file()

2 participants