Add span-based I/O wrappers as core/fxcrt/span_io.h [pdfium : main]
Tom Sepez (Gerrit)
Tom Sepez voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Commit-Queue | +1 |
3 comments
if (!::ReadFile(hFile, s.data(), static_cast<DWORD>(s.size_bytes()),`static_cast<DWORD>` could silently truncate if `s.size_bytes()` exceeds 4GB (i.e. larger than `MAXDWORD`). Consider using `pdfium::base::checked_cast<DWORD>` (from `core/fxcrt/numerics/safe_conversions.h`) to fail safely if such large buffers are ever passed. The same applies to `WriteFile` below.
if (bytes_read <= 0) {If `s` is empty (i.e. `s.size_bytes() == 0`), `read()` will return 0, which triggers this `<= 0` check and returns an empty span. This works, but passing a null pointer (which a default-constructed span might have) to `read()` can technically result in `EFAULT` on some OSs. You might want to add an early return if `s.empty()`. The same applies to `spanwrite`, `ReadFile`, and `WriteFile`.
// Bounds-checked writes from a container into a file. Returns whether theThe comment says "Returns whether the entire container was successfully written." which implies a boolean return type, but the function returns `size_t` representing the number of elements written. The same documentation mismatch exists for the POSIX and Windows overloads below.
Related details
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 3 comments![]( "Open in Gerrit")
Note that CFX_FileAccess_Posix may have fixed a latent error"CFX_FileAccess_Posix changes"
auto read_span = fxcrt::spanread(pdfium::span(buf), f);Explicit span construction not needed?
Related details
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 7 comments![]( "Open in Gerrit")
inline pdfium::span<T> spanread(Container&& dst, HANDLE hFile) {More hHandles.
inline pdfium::span<T> spanread(Container&& dst, FILE* pFile) {C++ style naming.
FILE* f = tmpfile();Add scopers to wrap all the different types of temp file creation + cleanup.
auto src_span = pdfium::span(This didn't need UNSAFE_TODO()?
Related details
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 10 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Commit-Queue | +1 |
10 comments
Note that CFX_FileAccess_Posix may have fixed a latent errorTom Sepez"CFX_FileAccess_Posix changes"
Done
inline pdfium::span<T> spanread(Container&& dst, HANDLE hFile) {Tom SepezMore hHandles.
Done
inline pdfium::span<T> spanread(Container&& dst, FILE* pFile) {Tom SepezC++ style naming.
Done
Add scopers to wrap all the different types of temp file creation + cleanup.
Done
Create closer to where used?
Nah, create after we just validated out_len.
This didn't need UNSAFE_TODO()?
I think the test directory is exempted by the config file. added anyway.
Explicit span construction not needed?
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |