Tolerate unintialized list in aws_array_list_length #1019
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1019 +/- ##
==========================================
+ Coverage 82.03% 82.05% +0.02%
==========================================
Files 52 52
Lines 5649 5650 +1
==========================================
+ Hits 4634 4636 +2
+ Misses 1015 1014 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| int result = aws_array_list_get_at_ptr(options->query_params, (void **)&uri_param_ptr, i); | ||
| AWS_FATAL_ASSERT(result == AWS_OP_SUCCESS); |
There was a problem hiding this comment.
Lint started complaining that uri_param_ptr can be NULL and we are dereferencing it. However, it cannot be NULL as index will always be valid.
Logs: https://github.com/awslabs/aws-c-common/actions/runs/4788915778/jobs/8516145068?pr=1019
| # if defined(AWS_WIN_ABORT_POP_UP_DISABLE) | ||
| _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); | ||
| # endif |
There was a problem hiding this comment.
| # if defined(AWS_WIN_ABORT_POP_UP_DISABLE) | |
| _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); | |
| # endif | |
| _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); |
I vote that we simply always do this.
Get rid of the AWS_WIN_ABORT_POP_UP_DISABLE cmake option, and cmakedefine, and --cmake-extra, and comment explaining why we need it in every ci.yml...
If we always set it, you'll still break in a debugger, if you have a debugger attached, see the implementation of aws_fatal_assert(), which calls aws_debug_break() which breaks into debugger if a debugger is attached, before calling abort().
There was a problem hiding this comment.
The only advantage of Windows' weird abort() popup is that it helps you break into a debugger. But no other platform does this, which is why we wrote our own logic to break into the debugger, which makes the popup totally useless for our tests...
always suppressing it will make tests on windows work like they do on all our other platforms
| #ifdef _MSC_VER | ||
| _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); | ||
| #endif |
There was a problem hiding this comment.
During my local testing, I found out that MinGW fails to link with _set_abort_behavior and doesn't have the pop-up behavior.
Issue #, if available:
Description of changes:
Follow up of Add a test for invalid cert aws-c-io#567 (comment)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.