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 possible buffer overflow #275

Closed
wants to merge 18 commits into from

Conversation

kamahen
Copy link

@kamahen kamahen commented Dec 22, 2023

This PR should be applied to branch develop.

These changes are because the compiler pointed out mis-use of strncmp() and memcpy(). I think that I've fixed the problems, but it's easy to make an off-by-one error in such code, so please review.

(A better way of fixing this would be to use std::string or std::basic_string<unsigned char>)

@kamahen
Copy link
Author

kamahen commented Dec 22, 2023

The files to check are:

libhdt/src/libdcs/CSD_FMIndex.cpp
libhdt/src/libdcs/CSD_HTFC.cpp
libhdt/src/libdcs/CSD_PFC.cpp
libhdt/src/triples/TripleListDisk.cpp

The other files are straightforward responses to compiler warnings.

@kamahen
Copy link
Author

kamahen commented Dec 25, 2023

The code now compiles cleanly using gcc-13.2 with -Wall -Wextra. There were some additional problems with the use of strncpy() that I fixed. (And, in an earlier commit, I hadn't correctly fixed a call to strncpy() -- I think it's correct now).
None of these compiler warnings resulted in any failures of the test cases, so I have no easy way of verifying that I've fixed anything, nor that I've fixed things correctly.

@kamahen
Copy link
Author

kamahen commented Mar 7, 2024

This is ready to merge into branch develop. (using this? develop...JanWielemaker:hdt-cpp:develop-buffer)

Or I can do the merge on my local copy and push to branch develop ... it's up to you. (A 2nd pair of eyes, looking at my changes, is always appreciated)

Once that is done, I'll create a PR for the hdt pack that references this.

@kamahen
Copy link
Author

kamahen commented Jun 25, 2024

I reviewed my changes and realize that there's a mistake in the calls to strncpy(). I will fix this and push a new commit.

@kamahen
Copy link
Author

kamahen commented Jun 28, 2024

I've looked at the 5 conflicts, and in each case, the first one ("develop-buffer") should be chosen (except, you might choose to ignore my "TODO").

I suspect that there are other potential buffer overflows in the code -- it would have been better if C++ std::vector or std::string had been used instead of C-style char* (especially with null-termination), but it appears that retro-fixing this is a non-trivial job, and it also appears that this project is no longer being maintained.

@kamahen
Copy link
Author

kamahen commented Jul 11, 2024

This PR has been superseded by #283 (same changes but no bogus merge conflicts).

@kamahen kamahen closed this Jul 11, 2024
@kamahen kamahen deleted the develop-buffer branch July 11, 2024 23:23
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.

4 participants