fix #4569: clang warning about deprecated sprintf usage. #4570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Here is the related issue: #4569 |
ckeshava
commented
Jun 12, 2023
src/ripple/json/impl/json_reader.cpp
Outdated
| @@ -925,7 +924,15 @@ Reader::getLocationLineAndColumn(Location location) const | |||
| int line, column; | |||
| getLocationLineAndColumn(location, line, column); | |||
| char buffer[18 + 16 + 16 + 1]; | |||
| sprintf(buffer, "Line %d, Column %d", line, column); | |||
| int generatedStrLen = | |||
| snprintf(buffer, 51, "Line %d, Column %d", line, column); | |||
There was a problem hiding this comment.
here is the documentation for the snprintf function: https://cplusplus.com/reference/cstdio/snprintf/
I'm basing the second parameter off of the previous line. But I don't understand why buffer needs to have a length of 50. The contents of buffer seem to require much lesser space.
a-noni-mousse
suggested changes
Jun 14, 2023
src/ripple/json/impl/json_reader.cpp
Outdated
Comment on lines
926
to
936
| char buffer[18 + 16 + 16 + 1]; | ||
| sprintf(buffer, "Line %d, Column %d", line, column); | ||
| int generatedStrLen = | ||
| snprintf(buffer, 51, "Line %d, Column %d", line, column); | ||
|
|
||
| if (generatedStrLen < 0) | ||
| { | ||
| ripple::LogicError( | ||
| "json_reader: Exception in " | ||
| "getLocationLineAndColumn - Could not write into buffer\n"); | ||
| } | ||
| return buffer; |
There was a problem hiding this comment.
Lets not use the ugly and dangereous C api. You can remove all lines and just put in return "Line " + std::to_string(line) + ", Colunn " + std::to_string(column)
There was a problem hiding this comment.
okay, thanks for the suggestion! I like this much better!
a-noni-mousse
approved these changes
Jun 20, 2023
Instead of sprintf use the suggested snprintf function
|
for next week: check if seelabs will be able to review this |
seelabs
suggested changes
Jul 10, 2023
| sprintf(buffer, "Line %d, Column %d", line, column); | ||
| return buffer; | ||
| return "Line " + std::to_string(line) + ", Column " + | ||
| std::to_string(column); |
There was a problem hiding this comment.
This does multiple allocations. Consider using snprintf instead. Something like:
int line, column;
getLocationLineAndColumn(location, line, column);
std::size_t n = 18 + 16 + 16 + 1;
char buffer[n];
snprintf(buffer, n, "Line %d, Column %d", line, column);
return buffer;
There was a problem hiding this comment.
Okay. I tried to do this in my first commit. But @a-noni-mousse had reservations with this approach (#4570 (comment))
Should I still go ahead with the snprintf approach?
There was a problem hiding this comment.
This isn't a performance critical function, the extra allocations aren't that big a deal. On the other hand, I don't find the snprintf dangerous here - it's clearly not going to overwrite the buffer and it's very easy to see that. So the issues I'd weigh are:
- 4 memory allocations vs 1 memory allocation
- a high level implementation vs a low level implementation
- this is not a performance critical function, and is unlikely to ever be performance critical
It's your call, I'll approve this with either interface.
There was a problem hiding this comment.
Hmm okay, thanks for the comparison.
I feel the snprintf adds complexity. I don't understand why we need to allocate 18 + 16 + 16 + 1 as the buffer size (I mimicked the existing code's behavior)
I'm inclined the favor the std::string version for it's simplicity.
seelabs
approved these changes
Jul 10, 2023
Instead of sprintf use the suggested snprintf function
Fix a bug in the `NODE_SIZE` auto-detection feature in `Config.cpp`. Specifically, this patch corrects the calculation for the total amount of RAM available, which was previously returned in bytes, but is now being returned in units of the system's memory unit. Additionally, the patch adjusts the node size based on the number of available hardware threads of execution.
- Resolve gcc compiler warning:
AccountObjects.cpp:182:47: warning: redundant move in initialization [-Wredundant-move]
- The std::move() operation on trivially copyable types may generate a
compile warning in newer versions of gcc.
- Remove extraneous header (unused imports) from a unit test file.
* Add release notes
* Enable api_version 2, which is currently in beta. It is expected to be marked stable by the next stable release. * This does not change any defaults. * The only existing tests changed were one that set the same flag, which was now redundant, and a couple that tested versioning explicitly.
…F#4512) Curtail the occurrence of order books that are blocked by reduced offers with the implementation of the fixReducedOffersV1 amendment. This commit identifies three ways in which offers can be reduced: 1. A new offer can be partially crossed by existing offers, so the new offer is reduced when placed in the ledger. 2. An in-ledger offer can be partially crossed by a new offer in a transaction. So the in-ledger offer is reduced by the new offer. 3. An in-ledger offer may be under-funded. In this case the in-ledger offer is scaled down to match the available funds. Reduced offers can block order books if the effective quality of the reduced offer is worse than the quality of the original offer (from the perspective of the taker). It turns out that, for small values, the quality of the reduced offer can be significantly affected by the rounding mode used during scaling computations. This commit adjusts some rounding modes so that the quality of a reduced offer is always at least as good (from the taker's perspective) as the original offer. The amendment is titled fixReducedOffersV1 because additional ways of producing reduced offers may come to light. Therefore, there may be a future need for a V2 amendment.
Enhance the /crawl endpoint by publishing WebSocket/RPC ports in the server_info response. The function processing requests to the /crawl endpoint actually calls server_info internally, so this change enables a server to advertise its WebSocket/RPC port(s) to peers via the /crawl endpoint. `grpc` and `peer` ports are included as well. The new `ports` array contains objects, each containing a `port` for the listening port (number string), and an array `protocol` listing the supported protocol(s). This allows crawlers to build a richer topology without needing to port-scan nodes. For non-admin users (including peers), the info about *admin* ports is excluded. Also increase test coverage for RPC ServerInfo. Fix XRPLF#2837.
There is now an Artifactory (thanks @shichengripple001 and team!) to hold dependency binaries for the builds. * Rewrite the `nix` workflow to use it and cut the time down to a mere 21 minutes. * This workflow should continue to work (just more slowly) for forks that do not have access to the Artifactory.
Apply a minor cleanup in `TypedField`: * Remove a non-working and unused move constructor. * Constrain the remaining constructor to not be overly generic enough as to be used as a copy or move constructor.
Introduces: * AccountRoot flag: lsfAllowClawback * New Clawback transaction * More info on clawback spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-39d-clawback
Replace hand-rolled code with std::from_chars for better maintainability. The C++ std::from_chars function is intended to be as fast as possible, so it is unlikely to be slower than the code it replaces. This change is a net gain because it reduces the amount of hand-rolled code.
Fix the libxrpl library target for consumers using Conan. * Fix installation issues and update includes. * Update requirements in the Conan package info. * libxrpl requires openssl::crypto. (Conan is a software package manager for C++.)
Remove deprecated fields from the ledger command: * accepted * hash (use ledger_hash instead) * seqNum (use ledger_index instead) * totalCoins (use total_coins instead) Update SHAMapStore unit tests to use `jss:ledger_hash` instead of the deprecated `hash` field. Fix XRPLF#3214
Rename `ServerHandlerImp` to `ServerHandler`. There was no other ServerHandler definition despite the existence of a header suggesting that there was. This resolves a piece of historical confusion in the code, which was identified during a code review. The changes in the diff may look more extensive than they actually are. The contents of `impl/ServerHandlerImp.h` were merged into `ServerHandler.h`, making the latter file appear to have undergone significant modifications. However, this is a non-breaking refactor that only restructures code.
* Commits 0b812cd (XRPLF#4427) and 11e914f (XRPLF#4516) conflict. The first added references to `ServerHandlerImp` in files outside of that class's organizational unit (which is technically incorrect). The second removed `ServerHandlerImp`, but was not up to date with develop. This results in the build failing. * Fixes the build by changing references to `ServerHandlerImp` to the more correct `ServerHandler`.
Certain inputs for the AccountTx method should return an error. In other words, an invalid request from a user or client now results in an error message. Since this can change the response from the API, it is an API breaking change. This commit maintains backward compatibility by keeping the existing behavior for existing requests. When clients specify "api_version": 2, they will be able to get the updated error messages. Update unit tests to check the error based on the API version. * Fix XRPLF#4288 * Fix XRPLF#4545
- Use powers of two to clearly indicate the bitmask - Replace bitmask with explicit if-conditions to better indicate predicates Change enum values to be powers of two (fix XRPLF#3417) XRPLF#4239 Implement the simplified condition evaluation removes the complex bitwise and(&) operator Implement the second proposed solution in Nik Bougalis's comment - Software does not distinguish between different Conditions (Version: 1.5) XRPLF#3417 (comment) I have tested this code change by performing RPC calls with the commands server_info, server_state, peers and validation_info. These commands worked as expected.
Small quality-of-life improvements to workflows using new concurrency control features: https://docs.github.com/en/actions/using-jobs/using-concurrency At time of this commit, macOS runners are oversubscribed. This may help.
The debug packages were named with the extension ".ddeb", but due to a
bug in Artifactory, they need to have the ".deb" extension. Debug symbol
packages with ".ddeb" extensions are not indexed, and thus are not
visible in apt clients.
* Fix the issue by renaming the debug packages in the build script.
* Use GCC-11 and update GCC Conan profile.
* This software requires GCC 11 and C++20. However, reporting mode is
built with C++17.
This is a quick band-aid to fix the build. Later, it will be better to
remove this package-building code.
For context, a Debian (deb) package contains bundled software and
resources necessary for installing and managing software on a
Debian-based system, including Ubuntu and derivatives.
When requesting `account_info` with an invalid `signer_lists` value, the API should return an "invalidParams" error. `signer_lists` should have a value of type boolean. If it is not a boolean, then it is invalid input. The response now indicates that. * This is an API breaking change, so the change is only reflected for requests containing `"api_version": 2` * Fix XRPLF#4539
* Update the version of the checkout action (for GitHub Actions) in
`clang-format.yml` and `levelization.yml`.
* The previous version, v2, was raising deprecation warnings due to
its reliance on Node.js 12.
* The latest checkout action version, v3, uses Node.js 16.
* Add a new YAML file (.pre-commit-config.yaml) to set up pre-commit
hook for clang-format
* The pre-commit hook is opt-in and needs to be installed in order to
run automatically
* Update CONTRIBUTING.md with instructions on how to set up and use the
clang-format linter
Automating the process of running clang-format before committing code
helps to save time by removing the need to fix formatting issues later.
This is a tooling improvement and doesn't change C++ code.
Enhance security during the build process:
* The '-fstack-protector' flag enables stack protection for preventing
buffer overflow vulnerabilities. If an attempt is made to overflow the
buffer, the program will terminate, thus protecting the integrity of
the stack.
* The '-Wl,-z,relro,-z,now' linker flag enables Read-only Relocations
(RELRO), a feature that helps harden the binary against certain types
of exploits, particularly those that involve overwriting the Global
Offset Table (GOT).
* This flag is only set for Linux builds, due to compatibility issues
with apple-clang.
* The `relro` option makes certain sections of memory read-only after
initialization to prevent them from being overwritten, while `now`
ensures that all dynamic symbols are resolved immediately on program
start, reducing the window of opportunity for attacks.
* Update the `account_info` API so that the `allowClawback` flag is
included in the response.
* The proposed `Clawback` amendement added an `allowClawback` flag in
the `AccountRoot` object.
* In the API response, under `account_flags`, there is now an
`allowClawback` field with a boolean (`true` or `false`) value.
* For reference, the XLS-39 Clawback implementation can be found in
XRPLF#4553
Fix XRPLF#4588
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
- This is an error-prone interface as it is easy to ignore when
overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
of ignoring overflow.
- It also allows for the use of optional::value_or() as a way to
explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
satisfy gcc's numeric_limits incomplete_type requirement.
Fix XRPLF#3495
---------
Co-authored-by: John Freeman <[email protected]>
…F#4552) Improve error handling for ledger_entry by returning an "invalidParams" error when one or more request fields are specified incorrectly, or one or more required fields are missing. For example, if none of of the following fields is provided, then the API should return an invalidParams error: * index, account_root, directory, offer, ripple_state, check, escrow, payment_channel, deposit_preauth, ticket Prior to this commit, the API returned an "unknownOption" error instead. Since the error was actually due to invalid parameters, rather than unknown options, this error was misleading. Since this is an API breaking change, the "invalidParams" error is only returned for requests using api_version: 2 and above. To maintain backward compatibility, the "unknownOption" error is still returned for api_version: 1. Related: XRPLF#4573 Fix XRPLF#4303
Sections that were rewrapped were wrapped to 72 characters, the same as the recommendation for commit messages.
|
Okay, I messed up the git commands. I want to sign the first "unverified" commit. But I have already merged the latest develop into my feature branch. I did a rebase and then |
|
That's because you rebased all of the subsequent commits. Let's meet up this week and try to fix this :) This Thursday @ 5:30pm should be fine |
|
@ckeshava : can you resolve the conflict? |
|
apologies, I made a mess out of the commit history. I have created a cleaner PR here: #4747 |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fix is intended to remove a warning from the clang compiler. It switches the usage of
sprintffunction into the recommendedsnprintffunction.Fix #4569
High Level Overview of Change
Context of Change
Type of Change