Change ip header parsing/header reporting #2503
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2503 +/- ##
============================================
+ Coverage 76.71% 78.71% +1.99%
Complexity 267 267
============================================
Files 138 112 -26
Lines 17641 13459 -4182
Branches 1034 0 -1034
============================================
- Hits 13533 10594 -2939
+ Misses 3571 2865 -706
+ Partials 537 0 -537
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
cataphract
force-pushed
the
glopes/x-amzn-trace-id
branch
from
2d5affa
to
30f6c6a
Compare
cataphract
changed the title
cataphract
force-pushed
the
glopes/x-amzn-trace-id
branch
2 times, most recently
from
f94a7e2
to
ce70294
Compare
appsec/src/extension/ip_extraction.c
Outdated
| memcpy(out, &cur, sizeof *out); | ||
| return EXTRACT_SUCCESS_PUBLIC; | ||
| } | ||
| // else is private |
There was a problem hiding this comment.
I think the previous conditional is readable enough that this comment is not adding anything
appsec/src/extension/ip_extraction.c
Outdated
| @@ -17,6 +17,7 @@ | |||
| #include <arpa/inet.h> | |||
There was a problem hiding this comment.
We manually keep this script aligned with ext/ip_extraction.c so I think you should add this new header there as well
There was a problem hiding this comment.
hm is there any reason to have this duplicated rather than in only one place?
There was a problem hiding this comment.
IIRC when we implemented this we were still in separate repositories and APM needs to collect the client IP using the same strategy when ASM is disabled but DD_TRACE_CLIENT_IP_ENABLED is set to true. Feel free to change it to a common implementation.
cataphract
force-pushed
the
glopes/x-amzn-trace-id
branch
2 times, most recently
from
3b09812
to
f2c81d0
Compare
cataphract
force-pushed
the
glopes/x-amzn-trace-id
branch
from
3980209
to
42fd16a
Compare
BenchmarksBenchmark execution time: 2024-02-19 15:07:17 Comparing candidate commit cb94096 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 0 performance improvements and 0 performance regressions! Performance is the same for 42 metrics, 50 unstable metrics. |
| @@ -89,6 +91,12 @@ static void dd_trace_load_symbols(void) | |||
| dlerror()); // NOLINT(concurrency-mt-unsafe) | |||
| } | |||
|
|
|||
| _ddtrace_ip_extraction_find = dlsym(handle, "ddtrace_ip_extraction_find"); | |||
There was a problem hiding this comment.
For the backlog but we're reaching the point where it would be useful to have some macros to do this and generate the relevant passthrough functions as well.
ext/ip_extraction.c
Outdated
| @@ -418,11 +458,11 @@ static bool dd_is_private_v6(const struct in6_addr *addr) { | |||
| static const struct { | |||
| union { | |||
| struct in6_addr base; | |||
| uint64_t base_i[2]; | |||
| unsigned __int128 base_i; | |||
There was a problem hiding this comment.
I believe @bwoebi had some concerns about the __int128:
bwoebi
force-pushed
the
glopes/x-amzn-trace-id
branch
from
42fd16a
to
c73bb45
Compare
ext/user_request.c
Outdated
| @@ -123,12 +123,17 @@ PHP_FUNCTION(DDTrace_UserRequest_notify_commit) | |||
| } | |||
| } | |||
|
|
|||
| if (status < 100 || status > 599) { | |||
| zend_type_error("Status code must be between 100 and 599"); | |||
There was a problem hiding this comment.
Can we have zend_value_error here and polyfill it in compatibility.h with #define zend_value_error zend_type_error - this was added with PHP 8 and existing "misuses" of TypeError replaced by it in php-src.
See APPSEC-51370 APPSEC-51531
Description
Reviewer checklist