Directory traversal support #830
Conversation
…e-system traversal with posix implementation.
include/aws/common/file.h
Outdated
| enum aws_file_type { | ||
| AWS_FILE_TYPE_NONE, | ||
| AWS_FILE_TYPE_FILE, | ||
| AWS_FILE_TYPE_SYM_LINK, |
There was a problem hiding this comment.
How about we get rid of this enum and just add aws_directory_entry_is_file(), _is_directory(), _is_symlink(), etc. Since an entry can be symlink AND file, or symlink AND directory.
That's the approach taken by std::filesystem::directory_entry
… The directory traversal is now a depth-first post-order traversal.
… change. Fixed affected tests or deleted them.
JonathanHenson
changed the title
| * Returns at least `size` of memory ready for usage. In versions v0.6.8 and prior, this function was allowed to return | ||
| * NULL. In later versions, if allocator->mem_acquire() returns NULL, this function will assert and exit. To handle | ||
| * conditions where OOM is not a fatal error, allocator->mem_acquire() is responsible for finding/reclaiming/running a | ||
| * GC etc...before returning. | ||
| */ | ||
| AWS_COMMON_API | ||
| void *aws_mem_acquire(struct aws_allocator *allocator, size_t size); |
There was a problem hiding this comment.
Food for thought: Do we want to let users register a global callback that fires before we terminate?
In the Herb Sutter paper that kicked off this whole discussion, it's mentioned as something they might do, in the theoretical world where C++ went this route:
We can additionally allow the program to register a global function to allow customizing what happens when
exiting each function body with an error, intended to be used only by the owner of the whole program. This is
useful for several purposes:
• To integrate with an existing system’s error handling or logging policy.
There was a problem hiding this comment.
Man, I love that paper:
Microsoft Word. Word also had been intentionally written to handle allocation failure gracefully. Re-
cently (summer 2019), the Word team conducted similar allocation fault injection test using their own
low-memory test environment. It discovered that the code they believed was allocation failure-resilient
actually failed pervasively when allocation failure was actually encountered; for example, 98% of execu-
tions crashed if <5MB memory was available. Examination showed that the code was doing a consist-
ently good job of checking for failures, but was consistently unable to meaningfully handle them. As an
experiment, the team built Word to unconditionally terminate on allocation failure, gradually rolled it
out to larger and larger subsets of users, and then made it universal in for the production release; there
was no statistically significant change in crash rates on Windows, Mac, iOS, or Android. The net result
was that Word now has fewer untested (and security-exploitable) error code paths, clearer code, and
improvements to execution performance and binary size because of elimination of “dead code” logging
and recovery logic (without loss of functionality because that code was already unused and nonworking
in practice).
There was a problem hiding this comment.
we already have that hook in the allocator implementation? want one? define that allocator and use it. done
There was a problem hiding this comment.
I agree, no reason to add more callbacks to set.
| /** | ||
| * Absolute path to the entry from the current process root. | ||
| */ | ||
| struct aws_byte_cursor path; | ||
| /** | ||
| * Path to the entry relative to the current working directory. | ||
| */ | ||
| struct aws_byte_cursor relative_path; |
There was a problem hiding this comment.
if we're going to require users to pass aws_string* or const char * to our file APIs, we may as well make path and relative_path either aws_string * or const char * too
There was a problem hiding this comment.
don't need to though. I can guarantee they're properly constructed. This does nothing except for increase allocaton overhead for no good reason.
There was a problem hiding this comment.
the api and type system will already force users to pass an aws_string to turn around and call the other apis. I don't know, I see the argument on ergonomics, but I don't think the ergonomics make practical sense here unless we go rewrite the aws_string implementation.
…d the missed const char * mess from aws_fopen().
…different machine though.
| # define AWS_PANIC_OOM(mem, msg) \ | ||
| do { \ | ||
| if (!(mem)) { \ | ||
| fprintf(stderr, "%s: %s, line %d", msg, __FILE__, __LINE__); \ |
There was a problem hiding this comment.
This doesn't help anything with CBMC traces, we can remove it from here. Developer will be able to see the error message in the value of msg.
| * Returns at least `size` of memory ready for usage. In versions v0.6.8 and prior, this function was allowed to return | ||
| * NULL. In later versions, if allocator->mem_acquire() returns NULL, this function will assert and exit. To handle | ||
| * conditions where OOM is not a fatal error, allocator->mem_acquire() is responsible for finding/reclaiming/running a | ||
| * GC etc...before returning. | ||
| */ | ||
| AWS_COMMON_API | ||
| void *aws_mem_acquire(struct aws_allocator *allocator, size_t size); |
There was a problem hiding this comment.
I agree, no reason to add more callbacks to set.
include/aws/common/string.h
Outdated
| * | ||
| * returns NULL on failure. | ||
| */ | ||
| AWS_COMMON_API struct aws_string *aws_string_convert_to_wchar_str( |
There was a problem hiding this comment.
Seems like a bad idea to SOMETIMES put UTF-16 in an aws_string.
I'd prefer we add a new type like aws_wstring or aws_string16 or aws_utf16. We wouldn't need to have the full suite of functions we have for vanilla aws_string, just add enough to get by
…nto directory_support
|
|
||
| struct aws_string *w_file_path = aws_string_convert_to_wchar_str(aws_default_allocator(), file_path); | ||
| struct aws_string *w_mode = aws_string_convert_to_wchar_str(aws_default_allocator(), mode); | ||
| struct aws_wstring *w_file_path = aws_string_convert_to_wstring(aws_default_allocator(), file_path); |
There was a problem hiding this comment.
This is devilish, but I get it. We're going to have to be super careful with path handling on windows, ensuring that we always utf-8 encode it ourselves.
mem_acquire no longer allows returning NULL, Windows unicode support in aws_string.h, file system and directory implementations for windows and posix.
This function's implementation was removed in #830, but the declaration wasn't.
**Issue:** A C++ SDK user found a memory leak in the Windows TLS channel-handler code and submitted the following PR: #651 In evaluating this 2-line fix, I noticed that it sat in the middle of some (preexisting) sloppy error-handling. **Description of changes:** - Fix memory leak, as shown in @normanade's PR, by calling `FreeContextBuffer()`. - Change it so we **always** call `FreeContextBuffer()` after calling to `AcceptSecurityContext()`/`InitializeSecurityContextA()`. Previously we'd only free the buffer in certain circumstances, and were probably leaking. - Remove (often sloppy) error-handling that tries to deal with allocation failures of `aws_io_message`. Officially declare that message allocations can't fail. - NOTE: Ever since [this change in 2021](awslabs/aws-c-common#830), our allocators will never return `NULL`. They deal with OOM by immediately terminating the program. This was done because error-handling code for allocation failure makes everything so much more complex, and it's seldom tested. We tried for years to handle it. It's not worth the effort. - Fix some error-handling that forgot to delete the `aws_io_message` after checking its capacity - In the Windows TLS code, replace some `AWS_ASSERT()` about buffers being large enough with `AWS_FATAL_ASSERT()`. Similarly, add an `AWS_FATAL_ASSERT()` in one place that never checked or asserted before doing the copy. - I don't know enough about TLS or Windows APIs to say whether or not these deserve real error-handling. So I'm just preserving the status quo (but crashing instead of doing undefined behavior). Co-authored-by: @normanade
mem_acquire no longer allows returning NULL, implementation and tests of file-system operations
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.