Preserve sub-millisecond event timestamp precision - #7
Open
bmehta001 wants to merge 28 commits into
Open
Conversation
) GUID_t::operator< used a non-lexicographic chained-|| comparison with a `Data3 == other.Data3` typo (should be `<`): return Data1 < other.Data1 || Data2 < other.Data2 || Data3 == other.Data3 || (memcmp(Data4,...) < 0); Two defects: (1) the Data3 line uses == instead of <, so GUIDs differing only in Data3 compare equivalent; (2) the chained-|| form is not lexicographic and violates antisymmetry (e.g. {1,5,..} and {2,3,..} can report both a<b and b<a). Using this as the ordering for std::set<GUID_t> / std::map<GUID_t,...> (the default std::less calls it; the comment says it is "needed for maps") is undefined behavior -- container corruption, infinite loops, or crashes. GUID_t is a public header type (EventProperty.hpp), so SDK consumers hit this with default ordered containers. Replace with proper lexicographic comparison (Data1, then Data2, then Data3, then memcmp(Data4)). Test: GuidTests.OperatorLess_IsStrictWeakOrdering (added) fails on the old operator (c<d == false; std::set of 4 distinct GUIDs collapses to 3) and passes after the fix. Verified by building and running UnitTests on Linux (host): all 12 GuidTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…adBlob (microsoft#1492) Under USE_ONEDS_BOUNDCHECK_METHODS, ReadBlob assigned the errno_t return of MAT::BoundCheckFunctions::oneds_memcpy_s (0 == success) directly to a bool: bool result = MAT::BoundCheckFunctions::oneds_memcpy_s(...); so `result` is false on success and true on failure -- inverted. ReadBlob (and thus ReadFloat/ReadDouble, which call it) returns "failure" for every successful read, breaking decode of any payload containing a blob/float/double field whenever the SDK is built with bound-check methods. The non-boundcheck branch one line below already does it correctly: `bool result = (memcpy_s(...) == 0);`. Compare the result against 0, matching the memcpy_s branch. Validated (TDD) with a standalone reader round-trip compiled with -DHAVE_ONEDS_BOUNDCHECK_METHODS: ReadDouble of a written double returns false before the fix and true (correct value) after. The bug is gated behind the non-default USE_ONEDS_BOUNDCHECK_METHODS option; the default build is unaffected (the changed line is #ifdef'd out) and the existing bondlite CompactBinaryProtocolTests assert ReadBlob == true under that option. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Update EventPropertiesDecorator.hpp
Scrub IP addresses by default.
* Gate IP scrubbing behind CFG_BOOL_ENABLE_IP_SCRUBBING (default on)
The initial change set RECORD_FLAGS_EVENTTAG_SCRUB_IP unconditionally for
every event, forcing IP scrubbing on all SDK consumers in direct-upload
mode -- a breaking change for apps that need client IP (e.g. geo-location).
Make scrubbing the default but opt-out: the decorator sets the SCRUB_IP
record flag unless CFG_BOOL_ENABLE_IP_SCRUBBING is explicitly set to false.
record.flags is forwarded on the cross-platform/direct-upload path, so this
redacts client IP at the collector without relying on ext.metadata privacy
tags.
- ILogConfiguration.hpp: add CFG_BOOL_ENABLE_IP_SCRUBBING config key
- EventPropertiesDecorator.hpp: gate SCRUB_IP flag behind the config
- EventPropertiesDecoratorTests.cpp: add default-on, opt-out, explicit-enable
tests with a per-instance ConfigurableLogManager helper
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Cover SDK stats events and add wrapper-parity config keys for IP scrubbing
- Statistics.cpp: SDK statistics/metastats events bypass EventPropertiesDecorator,
so apply the same collector-side client-IP scrub (gated by
CFG_BOOL_ENABLE_IP_SCRUBBING, on by default) to those records too. Closes the
gap identified in PR review.
- LogConfigurationKey.java + ODWLogConfiguration.{h,mm}: expose
CFG_BOOL_ENABLE_IP_SCRUBBING ('enableIpScrubbing') to the Android (Java) and
Apple (Obj-C) wrappers for API parity.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Clarify CFG_BOOL_ENABLE_IP_SCRUBBING docstring (Copilot round 1)
The doc implied the setting only applies in direct-upload mode, but the scrub
flag is set for all events and modes. Clarify that the flag is honored by the
OneCollector direct-upload path while UTC mode applies its own client-privacy
handling. No behavior change -- the flag is intentionally mode-agnostic and is
ignored by the UTC pipeline.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address Copilot round 2 on microsoft#1161: self-contained header + default-config docs
- EventPropertiesDecorator.hpp: include ILogManager.hpp so the header is
self-contained for the ILogManager (m_owner) and ILogConfiguration types /
CFG_BOOL_ENABLE_IP_SCRUBBING used inline, instead of relying on the includer.
- Clarify CFG_BOOL_ENABLE_IP_SCRUBBING docs (C++ / Java / Obj-C): scrubbing is
applied unless explicitly set to false (on by default) and the key is not
present in the default configuration, so GetDefaultConfiguration() does not
surface it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address Copilot round 3 on microsoft#1161: extract on-wire record flags to a shared header
Move the RECORD_FLAGS_EVENTTAG_* on-wire bits out of EventPropertiesDecorator.hpp
into a dedicated decorators/RecordFlagConstants.hpp, exposed as static constexpr
std::int64_t in the MAT namespace (no longer #define macros). This lets the stats
pipeline reference RECORD_FLAGS_EVENTTAG_SCRUB_IP via the small shared header
instead of pulling in the full decorator header, and avoids macro pollution.
- New: lib/decorators/RecordFlagConstants.hpp
- EventPropertiesDecorator.hpp: include the shared header; drop the macros
- Statistics.cpp: include the shared header instead of EventPropertiesDecorator.hpp
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Harden OfflineStorage_Room against null JNI objects (no-crash on null m_room/element)
Defensive follow-up to the microsoft#1227 family of Android Room crashes ("java_object ==
null in call to GetObjectClass" under GetAndReserveRecords). microsoft#1417 fixed the
stale-local-ref root cause but left two null paths unguarded, which can still
hard-abort the host process:
* m_room is null when the Room DB failed to open or was torn down (the
destructor already guards with `if (s_vm && m_room)`, but ~10 other methods
dereferenced it unconditionally). Add `if (!m_room) return <fail>;` guards to
DeleteRecords(x2), GetAndReserveRecords, ReleaseRecords, StoreRecords,
DeleteSetting, StoreSetting, GetSizeInternal, GetRecordCount,
ResizeDbInternal, and GetRecords. GetSetting already had this guard.
* a null element in the getAndReserve/releaseRecords result array (observed to
be androidx.room-version sensitive) was passed to GetObjectClass. Guard both
loops: GetAndReserveRecords pops the frame and stops (the existing
index < limit path releases the rest for retry); ReleaseRecords skips it.
Turns a process-killing JNI abort into graceful degradation. No functional change
on the healthy path. Compiles on Android only (JNI/Room) -- validated by CI.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: note the androidx.room version the SDK is built against (microsoft#1227 follow-up)
cpp-start-android.md covered Room setup but said nothing about the Room version.
The GetAndReserveRecords native crash (microsoft#1227, Room-version-sensitive) and the
null-guard hardening in this PR make this worth documenting: the maesdk AAR
brings androidx.room transitively (pinned in maesdk/build.gradle, currently
2.8.4); since Gradle resolves one Room version app-wide, consumers should not
force a version below what the SDK is built against and should prefer aligning
on the bundled (or a compatible newer) version. Points at build.gradle as the
source of truth so the doc doesn't drift on future bumps.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Room: guard null JNIEnv in 5 JNI methods; skip releaseUnconsumed on null element
Address the latest Copilot review (6 comments) on
lib/offline/OfflineStorage_Room.cpp.
ConnectedEnv null-env guards (5): DeleteByToken, ReleaseRecords, DeleteSetting,
StoreSetting and GetRecords created ConnectedEnv env(s_vm) and dereferenced
env->... behind only an if(!m_room) guard. ConnectedEnv::operator! can report a
null JNIEnv (null s_vm / thread-attach failure), and sibling methods already
guard with if(!env); added the matching early return (void/false/records) so a
null env no longer crashes.
releaseUnconsumed on null element: the null-array-element path broke out and
fell through to releaseUnconsumed(selected, index). The Java
StorageRecordDao.releaseUnconsumed (pre-existing since 2020, commit c81d46a)
reads selected[0..unconsumed-1] ignoring the offset, so on the null path it
could index the null element and throw, or release the wrong rows. A
sawNullElement flag now skips releaseUnconsumed on that path; the reserved
records expire and are retried (no data loss), and the normal end-early path is
unchanged.
Validated: NDK aarch64-linux-android23 -fsyntax-only clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com>
Co-authored-by: bmehta001 <bmehta001@users.noreply.github.com>
Co-authored-by: Bhagirath Mehta <bhamehta@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nary-footprint reductions (microsoft#1475) * Add vcpkg-release-bump workflow to automate port version bumps On a published version release, open a PR to microsoft/vcpkg bumping the cpp-client-telemetry port (REF -> tag, recomputed SHA512, version, then x-add-version). Runs only on published, non-prerelease version tags (vX.Y.Z.W) or manual dispatch, and opens no PR when the port already matches the release. Requires repo variable VCPKG_FORK_REPO and secret VCPKG_BUMP_TOKEN. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump in-repo overlay port to v3.10.161.1 tag Repoint tools/ports/cpp-client-telemetry REF from the pre-release commit to the published v3.10.161.1 tag (SHA512 updated) for exact parity with the official microsoft/vcpkg port. Version was already 3.10.161.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(vcpkg): add manifest-mode overlay fallback for pre-registry installs building-with-vcpkg.md only told manifest-mode users to add `cpp-client-telemetry` to vcpkg.json, which fails with an unknown-port error until the port is accepted into the official vcpkg registry. Document the `vcpkg-configuration.json` `overlay-ports` fallback that points manifest mode at the in-repo overlay port, giving parity with the classic-mode `--overlay-ports` instructions already in the doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg-bump: address Copilot force-with-lease comment + drop docs note .github/workflows/vcpkg-release-bump.yml (Copilot): `git push --force-with-lease` could fail on reruns because a fresh clone has no remote-tracking ref for an already-existing bump branch, so the workflow couldn't refresh an open bump PR (contradicting the "force-pushed branch refreshes it" intent). Fetch the branch into refs/remotes/origin/${BR} (|| true on the first run, when it doesn't exist yet) before the force-with-lease push so the lease has a ref to compare against. Verified at .github/workflows/vcpkg-release-bump.yml:146-152. docs/building-with-vcpkg.md: remove the manifest-mode overlay-ports note added in 136e010, per maintainer request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update vcpkg docs: port is now live in the official registry The cpp-client-telemetry port was merged into the upstream vcpkg registry (microsoft/vcpkg#52316, version 3.10.161.1), so docs/building-with-vcpkg.md no longer needs the conditional "once the port is accepted" phrasing. - Intro: state the port is published in the official registry and consumable directly; drop the stale "build recipe / CONTROL file" wording (vcpkg uses vcpkg.json, and the port is registry-resolved now). - "Installing from the vcpkg registry": present tense, link to the upstream ports/cpp-client-telemetry directory. - "Installing from the overlay port": reframe as development-only (test local port changes or a newer SDK revision before they reach the registry). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Align overlay port with the merged upstream vcpkg port The cpp-client-telemetry port merged into microsoft/vcpkg (ports/cpp-client-telemetry) ships only portfile.cmake + vcpkg.json. Bring the in-repo overlay back in sync so testing the overlay validates exactly what is published. - Drop the custom 'usage' file and its install step in portfile.cmake. The two lines it printed (find_package(MSTelemetry CONFIG REQUIRED) + target_link_libraries ... MSTelemetry::mat) duplicate vcpkg's auto-generated heuristic usage, and the upstream port carries no usage file. - Reorder vcpkg.json dependencies to vcpkg format-manifest canonical (alphabetical) order; same dependency set, no resolution change. After this, the overlay portfile.cmake and vcpkg.json are byte-identical to the upstream port blobs (cfdab23 / a74cc08). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix vcpkg-bump PR existence check: jq null skipped PR creation Copilot review (round): the open-PR existence guard used --jq '.[0].number'. On the first run, when no PR exists yet, gh pr list returns [] and .[0].number evaluates to null, which gh prints as the literal string "null". [ -n "null" ] is true, so the workflow wrongly logged "An open PR already exists" and skipped 'gh pr create' -- the release-bump PR would never be opened on a clean run. Fix: --jq '.[0].number // empty' yields empty output when no PR exists (guard false -> PR created) and the PR number when one does (guard true -> skipped). Verified jq semantics (jq 1.x): '[] | .[0].number' -> null (prints "null"); '[] | .[0].number // empty' -> no output; '[{number:42}] | .[0].number // empty' -> 42. Confirmed at .github/workflows/vcpkg-release-bump.yml:158. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden vcpkg-bump workflow: graceful no-op on non-version tags, no token in URL Address Copilot round comments on the release-bump workflow. vcpkg-release-bump.yml:77 - non-version tag handling was contradictory: the message said "skipping" but the step ran exit 1, failing the workflow. A release published with a non-4-part tag (the SDK has historical 3-part tags like v3.3.8) would mark the automatic run red for what should be a no-op. Now: manual workflow_dispatch with a bad tag still fails loudly (user error), but the automatic release trigger emits a notice, sets a 'skip' output, and exits 0. All downstream steps are gated on steps.ver.outputs.skip != 'true'. vcpkg-release-bump.yml:100 - the PAT was embedded in the clone URL, which persists it in .git/config and risks leaking if git echoes the remote. Switch to 'gh auth setup-git' (writes a credential helper to the global gitconfig) plus a tokenless https clone; the later push step reuses that helper via its GH_TOKEN env. No token appears in any URL or on disk. Validated: workflow YAML parses (PyYAML) and every embedded run block passes 'bash -n'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg-bump workflow: pass release tag via env to prevent shell injection Code review flagged a GitHub Actions script-injection vector: the "Resolve tag and version" step interpolated the untrusted release tag directly into the run: shell, before any validation. A tag containing shell metacharacters (e.g. v1.0.0.0";id;") would execute at assignment time, before the version regex runs. Injected code could write to GITHUB_ENV/GITHUB_PATH, which persist into the later Clone and push/PR steps that carry the VCPKG_BUMP_TOKEN PAT, enabling token exfiltration. Fix: pass the tag values through env: (RELEASE_TAG/INPUT_TAG) and reference them as quoted shell variables (TAG="${RELEASE_TAG:-$INPUT_TAG}"). Env values are not parsed as shell, so metacharacters can no longer inject. All downstream steps already use the regex-validated steps.ver.outputs.* values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Enable function-level linking in the CMake build so consumers can dead-strip The compiler-flags block in CMakeLists.txt is wrapped entirely in `if(NOT MATSDK_USE_VCPKG_DEPS)`, and its MSVC branch only sets warning flags -- never /Gy. So the CMake/vcpkg build (the one packaged for downstream consumers) compiles every TU without function-level COMDATs, and referencing one symbol pulls the whole .obj into the consumer image. Add a block, applied in BOTH vendored and vcpkg modes, that splits functions/data into COMDATs/sections (MSVC /Gy /Gw; GCC/Clang -ffunction-sections -fdata-sections; AppleClang -ffunction-sections). This lets a consumer's linker dead-strip unreferenced SDK code (/OPT:REF + /OPT:ICF, --gc-sections, -dead_strip) and matches the MSBuild Release projects, which already enable FunctionLevelLinking + OptimizeReferences + EnableCOMDATFolding. No source or ABI change. Bundled into the vcpkg PR since it directly improves the footprint of the vcpkg-packaged library. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reword function-level-linking comment to justify the vcpkg-mode exception Copilot review: the new comment read as conflicting with the earlier "let the toolchain manage compiler flags" note. Clarify that these section/COMDAT flags are a deliberate exception -- they are not optimization/dependency choices the toolchain owns; the toolchain doesn't set them, and without them the vcpkg-packaged library links whole .obj files. No code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Consolidate section-splitting flags into the global block (drop REL_FLAGS dupes) Copilot review: the non-vcpkg REL_FLAGS already injected -ffunction-sections (and -fdata-sections for GNU), duplicating the new global block on Release builds. Remove them from REL_FLAGS so the global add_compile_options block is the single source of truth for section splitting across both dependency modes (it now also covers MSVC /Gy /Gw and AppleClang, which CI confirms build clean). No behavior change; eliminates redundant flags and future drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg: request sqlite3 without default features (drop json1) + docs The SDK uses SQLite only for offline event storage (plain tables/indexes, no JSON/FTS/RTREE/vtab), so the port now declares sqlite3 with default-features:false instead of pulling json1. This is the necessary floor for footprint-conscious consumers: vcpkg unions features across the dependency graph and ignores default-features:false on transitive deps, so without this the SDK's own edge forces json1 on and no consumer can opt out. Measured: a consumer that also sets {"name":"sqlite3","default-features":false} in its root manifest links ~52 KB smaller (SQLITE_OMIT_JSON) on x64-windows-static Release; the vcpkg integration test stays 10/10. - tools/ports/cpp-client-telemetry/vcpkg.json: sqlite3 default-features:false; port-version 1 (port-only change over the published 3.10.161.1#0) - docs/building-with-vcpkg.md: document the required root-manifest opt-out Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(vcpkg): add consumer linker dead-strip guidance; clarify json1 opt-out Address Copilot round comments on docs/building-with-vcpkg.md and document the footprint lever consumers actually need (validated end-to-end on a real downstream DLL, which shrank substantially once /OPT:REF,ICF were on). - Add "Enable linker dead-stripping" section: the SDK's /Gy /Gw only enable stripping; the consumer's link must set /OPT:REF + /OPT:ICF (with the /DEBUG gotcha that silently disables them) + /INCREMENTAL:NO, or --gc-sections / -dead_strip on GNU/Clang/Apple. Note static-link vs DLL-reexport caveat. - json1 section: clarify this is the in-repo OVERLAY port (registry port to follow upstream), addressing the "registry still pulls defaults" comment. - Reword the resolution explanation around vcpkg's union model instead of "ignores transitive default-features:false". Verified by dry-run: SDK edge opt-out alone keeps sqlite3[core,json1]; adding the same at the root yields sqlite3 (no json1); any edge requesting defaults restores json1 -- so the consumer must opt out at the root too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg(overlay): drop port-version The in-repo overlay is only used for local testing (overlays ignore the version database), and the vcpkg-release-bump workflow deletes port-version on every bump, so it served no purpose here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(vcpkg): precise /DEBUG wording for /OPT:REF,ICF Address Copilot comment on the linker-stripping guidance. Per the MSVC /OPT docs, /DEBUG changes the /OPT default from REF/ICF to NOREF/NOICF (it does disable them by default, contrary to the comment's premise). Reword to the exact behavior ("flips their default to off, /OPT:NOREF,NOICF") and note that /OPT:REF is also incompatible with incremental linking (hence /INCREMENTAL:NO). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * cmake: clarify AppleClang dead-strip comment (ld64 atomizes per symbol) Address Copilot comment: reword the AppleClang branch to state the actual mechanism -- clang emits .subsections_via_symbols on Mach-O, so ld64's -dead_strip removes unreferenced code at per-symbol (function) granularity without -ffunction-sections (which we add only for cross-toolchain consistency). -fdata-sections stays omitted due to the historical bitcode conflict. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build: hide non-public symbols on non-Windows (-fvisibility=hidden) Add -fvisibility=hidden -fvisibility-inlines-hidden for non-MSVC builds and make MATSDK_LIBABI = __attribute__((visibility("default"))) on GCC/Clang, so only the MATSDK_LIBABI-decorated public API (the 56 public classes + the EVTSDK_LIBABI C API in mat.h) is exported; SDK internals and the bundled sqlite3/zlib are hidden. Non-Windows analog of the __declspec(dllexport)-gated export on Windows and of /Gy + the consumer's /OPT:REF: a much smaller dynamic symbol table -> faster dynamic linking/loading, smaller shared binaries, and more inlining/dead-code elimination. No behavior change for static consumers (hidden symbols remain usable within the same link); for shared-lib builds it restricts exports to the public API. Validated (NDK aarch64): compiling EventProperties.cpp with the flag yields the decorated public methods (EventProperties::SetType/GetType) as GLOBAL DEFAULT while internals (EventPropertiesStorage) are WEAK HIDDEN -- 64 exported vs 300 hidden in that one TU. Full cross-platform validation (build a shared lib and link a separate consumer on Linux/macOS/iOS/Android to confirm no public symbol is missing) should run in CI before merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address Copilot round on microsoft#1475: clang-cl /Gw gating + workflow_dispatch input - CMakeLists.txt: if(MSVC) is also true for the ClangCL toolset, which supports /Gy but not /Gw. Apply /Gy for all MSVC-like compilers and gate /Gw to real cl.exe (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") so ClangCL builds don't choke on /Gw. - vcpkg-release-bump.yml: read the workflow_dispatch tag via github.event.inputs.tag (concurrency key + resolve step) so manual dispatches resolve the tag unambiguously. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Windows shared-lib: add a dllimport path and tie export decoration to linkage The SDK has no .def file, so __declspec(dllexport)/(dllimport) via MATSDK_LIBABI is the sole Windows export mechanism, but it only had export and static states -- no dllimport path for a consumer of a shared mat.dll. And the cmake build hard-defined MATSDK_SHARED_LIB=1 for every Win32 build regardless of BUILD_SHARED_LIBS, so a static build still decorated the public API with dllexport (which also re-exports SDK symbols from any consumer DLL that absorbs the static lib). - ctmacros.hpp: add a MATSDK_IMPORT_LIB branch -> __declspec(dllimport). - lib/CMakeLists.txt: drop the hard-coded MATSDK_SHARED_LIB=1; instead set it on the mat target by linkage. Shared: PRIVATE MATSDK_SHARED_LIB (export from the SDK) + INTERFACE MATSDK_IMPORT_LIB (carried by the installed MSTelemetry::mat target, so find_package() consumers get dllimport automatically). Static: MATSDK_STATIC_LIB so nothing is decorated. This makes the C++ public API safe to consume from a single shared mat.dll (and fixes the missing-dllimport gap). The MSBuild/.vcxproj projects are unaffected (they define MATSDK_SHARED_LIB themselves). Add docs/sharing-a-single-sdk-runtime.md: how to ship one shared mat runtime that multiple modules in a process import (stable C ABI recommended; C++ shared-DLL path with its ABI-matching requirements), per-port vcpkg linkage, single LogManager lifetime ownership, one-copy-on-the-loader-path, and validation. Validated: MSVC preprocessor expands MATSDK_LIBABI to dllexport / dllimport / empty for the shared / import / static cases; cmake configures and the static Linux build is unaffected (change is Windows-guarded). Windows shared export/import is exercised by CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Scope -fvisibility-inlines-hidden to C++ to avoid -Werror C failures -fvisibility-inlines-hidden is a C++-only option. Applying it to all languages via add_compile_options meant the legacy Android path's bundled C sources (sqlite3_bundled, zlib_bundled) received it too; under Clang this emits an unused-argument warning that becomes an error with the project's -Werror. Scope it to CXX via a COMPILE_LANGUAGE generator expression while keeping -fvisibility=hidden for both C and C++. Files changed: - CMakeLists.txt: -fvisibility-inlines-hidden gated to \$<COMPILE_LANGUAGE:CXX> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Re-export ObjC wrapper classes in shared Apple builds The global -fvisibility=hidden (root CMakeLists.txt) hides Objective-C class symbols (_OBJC_CLASS_\*) as well as C++ internals. With the default BUILD_OBJC_WRAPPER=YES, a shared libmat.dylib on Apple would therefore export none of the public ODW* wrapper classes, and consumers linking against them would fail with undefined _OBJC_CLASS_\... symbols. Collect the ObjC wrapper translation units into OBJC_WRAPPER_SRCS and, for shared Apple builds, compile just those units with -fvisibility=default so the public Objective-C API is re-exported while the C++ core stays hidden. Files changed: - lib/CMakeLists.txt: OBJC_WRAPPER_SRCS variable + per-source -fvisibility=default for shared Apple builds Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use portable OR generator expression in footprint docs snippet The multi-config form \$<CONFIG:Release,RelWithDebInfo> only matches on CMake >= 3.19; on older CMake it compares the literal string and never matches, so a consumer copy-pasting the snippet would silently fail to enable /OPT:REF, /OPT:ICF, and /INCREMENTAL:NO. Switch to \$<OR:\$<CONFIG:Release>,\$<CONFIG:RelWithDebInfo>>, which is valid across all supported CMake versions. Files changed: - docs/building-with-vcpkg.md: OR-based CONFIG generator expression in the dead-strip snippet Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Gate visibility on NOT WIN32; clarify C-API dllimport doc wording Two review fixes: - CMakeLists.txt: the hidden-visibility block is described as non-Windows but was gated on NOT MSVC, which also matches MinGW/Clang-GNU Windows and would apply ELF-style -fvisibility=hidden to a PE/COFF target. Gate it on NOT WIN32 so all Windows toolchains rely on __declspec(dllexport) as intended. - docs/sharing-a-single-sdk-runtime.md: the C-API bullet said it 'links without __declspec(dllimport)', which contradicted the new MATSDK_IMPORT_LIB interface define. Reworded to: dllimport is not required (a C function resolves via the import-lib thunk) but is applied automatically to shared consumers as a harmless optimization. Files changed: - CMakeLists.txt: NOT MSVC -> NOT WIN32 for the visibility block - docs/sharing-a-single-sdk-runtime.md: C-API dllimport wording Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Scope footprint guidance to static-link scenarios The 'Reducing binary footprint' section assumed the SDK is always linked statically, but vcpkg's default triplets (e.g. x64-windows) build dynamic libraries and the port also supports BUILD_SHARED_LIBS=ON. Clarify that the consumer-side dead-stripping guidance applies to static linkage, and note that a dynamic mat ships its own runtime whose export table is already trimmed by the SDK's -fvisibility=hidden and /Gy /Gw. Files changed: - docs/building-with-vcpkg.md: scope footprint section to static-link case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Gate non-Windows default-visibility on shared builds (mirror Windows) On non-Windows, MATSDK_LIBABI unconditionally expanded to __attribute__((visibility("default"))), so even in a static build the public API kept default visibility despite -fvisibility=hidden. A consumer that statically absorbs libmat.a into its own .so/.dylib would then unintentionally re-export the SDK's public API (larger dynamic symbol table, leaked SDK surface) -- the non-Windows analog of the Windows re-export that MATSDK_STATIC_LIB already prevents. Gate the visibility attribute on MATSDK_SHARED_LIB so it mirrors the __declspec(dllexport) gating: shared builds export the API; static builds omit the attribute, letting the public symbols inherit -fvisibility=hidden. Define MATSDK_SHARED_LIB PRIVATE for all shared builds in lib/CMakeLists.txt (keeping the Windows-only INTERFACE MATSDK_IMPORT_LIB and MATSDK_STATIC_LIB). Verified on Linux (readelf): static build -> evt_api_call_default is GLOBAL HIDDEN (still resolvable by static linking -- a consumer links+runs against libmat.a -- but not re-exported); shared build -> GLOBAL DEFAULT (exported from libmat.so). Files changed: - lib/include/public/ctmacros.hpp: non-Windows MATSDK_LIBABI gated on MATSDK_SHARED_LIB - lib/CMakeLists.txt: define MATSDK_SHARED_LIB for all shared builds Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Prepare for new release - 3.10.170.1 Regenerated lib/include/public/Version.hpp via tools/gen-version (date-derived version: 3.<(year-2020)+4>.<dayOfYear>.1 = 3.10.170.1). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update version to 3.10.173.1 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ropagation (microsoft#981), and docs (microsoft#1458) * Document supported Windows build toolsets Clarify the VS2019/VS2022/VS2026 command-line build entry points, avoid legacy .NET Framework 4.0 projects in VS2022+ wrappers, and make solution-level :Build targets work through RunMsBuild. Files changed: - docs/cpp-start-windows.md - build-all.bat - build-all-v143.bat - build-all-v145.bat - tools/RunMsBuild.bat - tools/setup-buildtools.cmd - tools/vcvars.cmd - tools/.vsconfig.vs2022 - tools/.vsconfig.vs2026 - Solutions/before.targets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Windows build script review comments Clarify the Windows Visual Studio build entry point by moving the build matrix implementation to build-all-windows.bat while keeping build-all.bat as a compatibility wrapper. Update version-specific wrappers, CI, helper scripts, and docs to call the clearer name. Also clarify that MFC/ATL Visual Studio components remain intentional because SampleCppMini uses static MFC. Validation: - git diff --check - build-all-v142.bat Solutions\build.compact-dll.props smoke with all build legs skipped confirmed custom props forwarding - material self-review found no remaining issues Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci(windows): make the VS2022 build gate fail correctly Addresses code-review findings on the new Windows build CI: - build-windows-vs2022.yaml ran build-all-windows.bat with v143/vs2022 but without SKIP_NET40_BUILD, so it tried to build the legacy net40 / SampleCsNet40 projects that VS2022 cannot build (and that the docs say are skipped). Add SKIP_NET40_BUILD: 1 to match the canonical build-all-v143.bat. - build-all-windows.bat called tools\RunMsBuild.bat ~12 times with no errorlevel check between them, so only the last build's exit code reached the caller; an intermediate config failure was swallowed and the gating CI job could report success on a broken build. Add 'if errorlevel 1 exit /b 1' after each call to fail fast. (Pre-existing in build-all.bat, but newly load-bearing now that a CI gate runs this script; verified the swallowing and the fix with an isolated batch repro.) - Remove continue-on-error: true from the Checkout step so a failed checkout fails the job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build(win): add concrete Windows 11 SDK 10.0.22621 to vs2022/vs2026 .vsconfig The vs2022/vs2026 component configs only listed the bare Microsoft.VisualStudio.Component.Windows10SDK; since setup-buildtools.cmd applies them with --quiet and no --includeRecommended, a fresh install could end up with no concrete Windows SDK and fail to build. Add the explicit Windows11SDK.22621 component (the 10.0.22621 platform SDK the repo's CodeQL workflow already pins via WindowsSDKVersion: 10.0.22621.0), mirroring how .vsconfig.vs2019 pairs the bare component with a concrete Windows10SDK.18362. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address Copilot review: doc project list + robust cd in build-all-windows.bat - docs/cpp-start-windows.md: drop 'win32-cs' from the list of legacy .NET 4.0 projects the VS2022/2026 wrappers skip. MSTelemetrySDK.sln only contains net40 and SampleCsNet40 (gated by SKIP_NET40_BUILD); win32-cs lives in a separate Solutions/win32-cs solution and is not part of the SDK build matrix. - build-all-windows.bat: use 'cd /d "%~dp0"' so the script reliably changes drive and tolerates spaces when invoked from another drive/working directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address Copilot review: locate sibling scripts via %~dp0, not CWD The VS-version wrappers (build-all-v142/v143/v145.bat) called build-all-windows.bat by bare relative name, and tools\build-Win10-compact-exp.cmd used 'cd ..' + a CWD-relative call. Invoked from another drive/working directory (e.g. via an absolute path), the sibling script wouldn't be found. Call it via a %~dp0-relative absolute path instead so the wrappers work from any CWD: - build-all-v142/v143/v145.bat: call "%~dp0build-all-windows.bat" %* - build-Win10-compact-exp.cmd: drop the fragile 'cd ..'/%CD% and call "%~dp0..\build-all-windows.bat" with the props file as a %~dp0-relative path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build(win): fail with a clear message when Visual Studio is not detected vcvars.cmd sets VSTOOLS_NOTFOUND and exits 0 when it can't find any Visual Studio install, but RunMsBuild.bat didn't check it and went straight to msbuild, so a developer with no/undetected VS only saw a cryptic "'msbuild' is not recognized". Check VSTOOLS_NOTFOUND after calling vcvars and print an actionable error (install VS with the C++ workload, or run setup-buildtools.cmd, or set VSTOOLS_VERSION) before aborting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build(win): warn when VS detection falls back to a different version vcvars.cmd's label cascade silently moves forward when the requested Visual Studio isn't installed (e.g. a vs2022 request ends up on vs2026). Combined with the version wrappers pinning PlatformToolset (v143/v145), that mismatch surfaces later as a confusing toolset error from msbuild. Capture the explicitly requested version and, once configuration succeeds, print a clear warning when the detected VS differs from what was asked for. The configured path now returns a deterministic exit 0 (callers key off VSTOOLS_NOTFOUND, not the exit code). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build(win): make VSTOOLS_NOTFOUND robust to stale shell state Copilot round: vcvars.cmd set VSTOOLS_NOTFOUND=1 on the not-found path but never cleared it on success, so a stale value left in the shell by a previous failed run could make RunMsBuild.bat's new guard abort a build even when VS is present. Clear VSTOOLS_NOTFOUND at vcvars.cmd entry (only the not-found path sets it now) and check the explicit ==1 value in RunMsBuild.bat instead of mere existence. Verified at tools/vcvars.cmd entry and tools/RunMsBuild.bat:17. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build(win): clear stale VS output vars at vcvars.cmd entry Copilot round: clearing only VSTOOLS_NOTFOUND wasn't enough. Callers such as tools\setup-buildtools.cmd gate on if exist "%VSINSTALLDIR%" and use %VSVERSION% for the .vsconfig path, so a stale VSINSTALLDIR/VSVERSION/VSDEVCMD left in the shell (or caller environment) could make them act on the wrong install after a failed detection. Reset VSINSTALLDIR, VSDEVCMD and VSVERSION alongside VSTOOLS_NOTFOUND at entry so each run starts from a fully clean detection state; only the matching detection path repopulates them. Verified: stale VSINSTALLDIR is replaced with the real path on success and left empty when detection doesn't match (tools/setup-buildtools.cmd:42,44). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ionData parse hardening) (microsoft#1179) * Add noexcept on non throwing methods identified by static analysis * LogSessionDataProvider: harden convertStrToLong (reset errno, reject negatives) Addresses the Copilot low-confidence (suppressed) review note on convertStrToLong: - strtoll's errno was checked without being reset first, so a stale errno from an earlier call could trip false "conversion failed" handling. errno is now cleared before the call. - a negative input wrapped silently into a large uint64_t. The value is now parsed into a signed temp and rejected (returns 0 + warns) if negative. - the imprecise res==LONG_MAX overflow heuristic is replaced by a direct errno==ERANGE check plus an explicit no-conversion / trailing-character check. noexcept is preserved (no throwing operations). Pre-existing logic, folded in here since this PR already annotates the same function. Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * LogSessionDataProvider: parse with strtoull to match uint64_t return type Follow-up to the Copilot review on convertStrToLong: the function returns uint64_t but parsed via std::strtoll (signed long long), which constrains the accepted range to LLONG_MAX and mixes signed/unsigned. Switch to std::strtoull so parsing matches the return type and the full uint64_t range is accepted. strtoull silently wraps a leading '-', so negatives are now rejected explicitly (first non-space char check) before parsing, preserving the earlier negative-rejection behavior. noexcept preserved. Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * LogSessionDataProvider: mark remove_eol noexcept remove_eol only inspects and shrinks the string in place (empty(), operator[], length(), and erase() at a validated position) -- none of which allocate or throw -- so it is genuinely non-throwing. Extends this PR's noexcept coverage to the one remaining sibling helper in the session-data classes that is safely non-throwing (parse()/writeFileContents()/the std::string ctor allocate, so they correctly stay un-annotated). Verified NDK aarch64-linux-android23 -fsyntax-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * EventProperties: add noexcept move constructor and move assignment EventProperties is pimpl (single EventPropertiesStorage* m_storage) with a user-declared copy ctor/assign + virtual dtor, so the compiler generated no move operations -- every pass/return/vector-growth deep-copied the whole property map via `new EventPropertiesStorage(*copy.m_storage)`. Add O(1) noexcept move ctor + move assignment that transfer the storage pointer. The dtor is already null-safe (delete nullptr); copy-assignment is now also null-safe so a moved-from object can be reassigned. Backward-compatible API addition (the single-pointer layout is unchanged). Verified: NDK aarch64-linux-android23 -fsyntax-only clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * LogSessionDataProvider: fix %d format specifier for const char* names The three failure-path LOG_WARN calls passed the static const char* setting names (sessionFirstLaunchTimeName/sessionSdkUidName) to a %d conversion, which is undefined behavior and logs a garbage integer instead of the setting name exactly when a store/delete failed. Line 71 already uses %s with .c_str(); make these three consistent with %s. Folded into this PR since it already hardens this file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com> Co-authored-by: Bhagirath Mehta <bhamehta@microsoft.com> Co-authored-by: bmehta001 <bmehta001@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…f copying (microsoft#1498) * Decorator: move CsProtocol::Value temporaries into the ext maps EventPropertiesDecorator builds a throwaway CsProtocol::Value (or the Part B map) for every event property and copy-assigned it into the ext/extPartB maps. Each value is a local that is not used after insertion, so move it instead of copying. CsProtocol::Value is a heavy type (vectors of attributes/PII plus strings), and this runs on the per-event decorate path. Pure move-instead-of-copy of throwaway locals; no interface or behavior change. EventProperties decorator/serialization tests (54) pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot comment: include <utility> for std::move The decorator now uses std::move; add the explicit <utility> include instead of relying on transitive includes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…icrosoft#1484) OfflineStorageTests_SQLite.InitializeDeletesFileAndCreatesNewIfFailed fails only on iOS: after a corrupt DB is detected, recreate() opens with deletePrevious=true, but the open then fails and OnStorageOpened reports "SQLite/None" instead of the expected "SQLite/Clean". Root cause hypothesis: deletePrevious only removed the main database file via the SQLite VFS xDelete. A stale -journal/-wal/-shm companion left behind by the failed first open can prevent the freshly created database from opening cleanly. This is benign on Windows/Linux (where the test passes) but trips the iOS VFS. Fix: in SQLiteWrapper::initialize's deletePrevious path, delete the main DB file plus its -journal/-wal/-shm companions. Only a failure to delete the main file is fatal; a leftover companion that cannot be removed no longer aborts the recreate, since the subsequent open may still succeed. No desktop regression: built UnitTests (Release x64, v145) and ran the full OfflineStorageTests_SQLite suite -- 43/43 pass on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…icrosoft#1483) * android: initialize EventPropertiesStorage.eventType to fix null getType() EventPropertiesStorage's default constructor initialized every member except eventType, so EventProperties.getType() returned null on a freshly constructed object instead of its documented empty-string default (microsoft#1329). On Android/Java this surfaced as a NullPointerException for callers. - EventPropertiesStorage(): initialize eventType = "" (matching eventName and the other members). - EventsUnitTest: add newEventPropertiesGetTypeReturnsEmptyString as a regression test (getType() on a new EventProperties is non-null and ""). Resolves microsoft#1329. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Don't forward an empty event type to native SetType (Android) Code review of the microsoft#1329 fix found a native-path regression. Making Java getType() return "" (instead of null) fixes the Java-side NPE, but the JNI converter forwarded the type to native whenever the jstring was non-null: if (jstrEventType != NULL) eventProperties.SetType(JStringToStdString(env, jstrEventType)); Before microsoft#1329, getType() returned null for a default EventProperties, so SetType was skipped and native eventType stayed at its "" default. Now the non-null "" reaches native SetType(""), which fails validateEventName (length < 4) and, for EVERY typeless event, logs "Invalid event type!" and broadcasts an EVT_REJECTED DebugEvent to all registered listeners (EventProperties.cpp SetType -> ILManager::DispatchEventBroadcast) -- a false-positive rejection signal indistinguishable from a genuinely rejected event. Fix: in the JNI converter (the sole native SetType chokepoint), treat an empty type as "unset" and only call SetType for a non-empty string, restoring the pre-microsoft#1329 native behavior while keeping the corrected Java API contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot on microsoft#1483: test EventPropertiesStorage directly (no native) The regression test constructed EventProperties, whose constructor calls setName() -> native Utils.validateEventName(). These are JVM unit tests (@RunWith(MockitoJUnitRunner)) with no native library loaded, so it would throw UnsatisfiedLinkError instead of exercising the regression. Test the pure-Java EventPropertiesStorage (same package) directly -- the exact class the microsoft#1329 fix initialized (eventType = ""). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rosoft#1480) * Build native desktop SDK in MSVC conformance mode (/permissive-) Enable ConformanceMode (/permissive-) for the native desktop SDK projects via the shared Solutions/build.props, so non-standard MSVC extensions are caught at build time (issue microsoft#255). Two project families are deliberately excluded, gated in the ItemDefinitionGroup condition: * UWP / Windows Store projects (AppContainerApplication=true) compile as C++/CX (/ZW), which MSVC rejects in combination with /permissive-. * C++/CLI managed projects (Keyword=ManagedCProj, e.g. net40) target the managed runtime and are a separate conformance domain. build.props is imported only by the SDK projects (net40, win10-*, win32-*), not by vendored sqlite/zlib or the test projects, so vendored third-party code is unaffected. Validated locally (VS 2026, v145, x64 Release): clean Rebuild of win32-lib (58 files) plus win32-dll / win32-mini-lib / win32-mini-dll all compile with 0 conformance errors and 0 warnings. Confirmed the gate excludes UWP (win10-lib does not receive /permissive-, so its /ZW build is unaffected). Files: Solutions/build.props Resolves microsoft#255. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Extend conformance mode to net40 (C++/CLI) and fix /permissive- errors Bring the net40 managed (C++/CLI) project under /permissive- by dropping the Keyword=ManagedCProj exclusion from the build.props gate, and fix the two conformance errors this surfaced in shared CX code: * lib/shared/EventPropertiesCX.cpp: add `typename` to the dependent name `map<string,T>::iterator` in StoreEventProperties (C3878/C2065 under two-phase name lookup). * lib/shared/PlatformHelpers.h: forward-declare FromPlatformString before the FromPlatformMap templates that call it. Under strict two-phase lookup the dependent call fell back to ADL (which searches Platform::, not the SDK namespace) and failed with C3861. Now only UWP / Windows Store (C++/CX, /ZW) projects are excluded, since /permissive- is incompatible with /ZW. Validated (VS 2026, v145, x64 Release): clean Rebuild of the full non-UWP SDK set -- win32-lib, win32-dll, win32-mini-lib, win32-mini-dll and net40 -- with 0 conformance errors. (net40 is .NET Framework 4.0 C++/CLI; local builds need the v4.0 reference-assembly targeting pack, which the SDK CI provides.) Files: Solutions/build.props, lib/shared/EventPropertiesCX.cpp, lib/shared/PlatformHelpers.h Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build: address Copilot round-1 — scope /permissive- to SDK library projects only Solutions/build.props (Copilot): the ConformanceMode block lived on the shared build.props, which is ALSO imported by vendored sqlite/zlib, the test projects (gtest/gmock/UnitTests/FuncTests) and the samples -- so /permissive- was applied far more broadly than intended, and the PR description's claim was wrong. Verified with `git grep -l build.props -- '*.vcxproj'`: sqlite/sqlite.vcxproj, third_party/.../zlibvc.vcxproj and tests/{functests,unittests}/*.vcxproj all import it. Fix: move ConformanceMode into a dedicated Solutions/conformance.props, imported explicitly only by the five native SDK library projects (win32-lib, win32-dll, win32-mini-lib, win32-mini-dll, net40), right after their build.props import so the setting wins. build.props is reverted to its original content. Validated (VS 2026, v145, x64 Release): /permissive- present on win32-lib's cl invocations (0 conformance errors); a standalone sqlite:Rebuild now shows 0 /permissive- occurrences; clean Rebuild of all five SDK library projects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er final + TU-local statics (microsoft#1488) * devirt: mark concrete leaf impl classes final Mark the leaf concrete implementations of IHttpClient and IOfflineStorage final so the compiler can devirtualize (and often inline) calls made through them: HttpClient_{Curl,WinInet,WinRt,Apple,Android,CAPI} and OfflineStorage_Room / MemoryStorage / OfflineStorageHandler. Each was verified to have no subclass anywhere in the tree (lib + tests + wrappers). Deliberately NOT marked: - OfflineStorage_SQLite -- tests/unittests/OfflineStorageTests_SQLite.cpp subclasses it (OfflineStorage_SQLiteNoAutoCommit). - TelemetrySystemBase -- base of TelemetrySystem / AITelemetrySystem. Validated: NDK aarch64 -fsyntax-only on OfflineStorage_Room, OfflineStorageHandler and MemoryStorage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Internal linkage: mark TU-local C-API helpers and DllMain global static Companion to the `final` devirtualization in this PR: give internal linkage to translation-unit-local symbols so the compiler can inline / drop them and keep them out of the (static-archive and shared-object) symbol table. This helps the static-lib consumption path that -fvisibility=hidden does not fully cover, since hidden visibility only trims the dynamic export table while these symbols keep external linkage across TUs. lib/api/capi.cpp: mark the 10 file-local C-API dispatch helpers static (remove_client, mat_open_core, mat_open, mat_open_with_params, mat_log, mat_close, mat_pause, mat_resume, mat_upload, mat_flushAndTeardown). Verified each is called only from the single exported entry point evt_api_call_default (and each other) within capi.cpp, and appears in no header and no other translation unit. capi_get_client stays external (it is MAT::capi_get_client, declared in a header and used by the CAPI HTTP client). Consistent with the file's existing static mtx/clients. lib/shared/dllmain.cpp: mark thread_count static -- a file-scope mutable global mutated only inside DllMain in this TU. get_platform_uuid (sysinfo_sources.cpp) was deliberately NOT touched: it has no caller in-tree, so marking it static would trip -Wunused-function under -Werror. Verified: NDK clang aarch64 -fsyntax-only on capi.cpp is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix typo in capi.cpp comment: Marashal -> Marshal Addresses Copilot review comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Narrow scope: keep 'final' only where it measurably devirtualizes Per review feedback (lalitb), measured the devirtualization effect of the 'final' markers (vendored Linux shared libmat.so, -O2 -ffunction-sections -fvisibility=hidden -Wl,--gc-sections, no LTO), base vs this PR: - Final binary size: NO change (.text byte-size identical). Devirtualization swaps an indirect call for a same-width direct call -- perf, not size. - 'final' devirtualizes only where the concrete type is known: object-level indirect-call counts dropped in OfflineStorageHandler.o (70->64) and MemoryStorage.o (8->3) -- a class calling its own virtual methods and OfflineStorageHandler's concrete MemoryStorage member. Call sites routed through IHttpClient/IOfflineStorage base references were unchanged (LogManagerImpl/TelemetrySystem/OfflineStorage_SQLite/HttpClient_Curl TUs byte-identical). So 'final' on the HttpClient_* clients and OfflineStorage_Room bought no measured devirtualization while still restricting subclassing. Drop it from those; keep it only on MemoryStorage and OfflineStorageHandler (internal lib/offline impl types, not extension points), where it does help. The internal-linkage (static) cleanups in capi.cpp and dllmain.cpp are orthogonal and retained. Files: lib/http/HttpClient_{Android,Apple,CAPI,Curl,WinInet,WinRt}.hpp, lib/offline/OfflineStorage_Room.hpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Drop redundant virtual on non-override methods in final storage classes Newer Clang (macOS-latest, LLVM 19+) enables -Wunnecessary-virtual-specifier, which errors under -Werror when a 'virtual' method that does not override a base method lives inside a 'final' class (it can never be overridden). Marking OfflineStorageHandler and MemoryStorage final left three such methods (DeleteRecordsByKeys, isKilled, GetReservedCount) still declared virtual, breaking the macOS debug build. Remove the now-redundant virtual specifier; these become non-virtual, consistent with the devirtualization goal. No subclasses or overrides of these methods exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…microsoft#1500) * Skip MultipleLogManagersTests on iOS (hangs the simulator CI job) The iOS CI build (build-ios-mac.yml -> iOSFuncTests) consistently sat until its 60-minute timeout. The hang point is MultipleLogManagersTests.ThreeInstancesCoexist: these tests stand up an in-process HttpServer on a loopback port and run multiple concurrent LogManager instances uploading to it, which deadlocks inside the iOS simulator sandbox (the log shows the test starting, a loopback 'Connection reset by peer', then no further output until the job is canceled at 60 minutes). Skip the whole fixture on iOS via GTEST_SKIP() in SetUp(), guarded by TARGET_OS_IPHONE so macOS and desktop targets keep exercising it. The guard is a no-op on non-Apple platforms (TARGET_OS_IPHONE undefined). Files: tests/functests/MultipleLogManagersTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * iOS: exclude MultipleLogManagersTests at compile time, not via GTEST_SKIP The first attempt skipped the suite via GTEST_SKIP() in SetUp(), which stopped the 60-minute hang but the iOS xctest gtest wrapper does not honor a SetUp skip: the test bodies still ran (with the HttpServer never started) and failed at MultipleLogManagersTests.cpp:183 and :221. Exclude the whole suite from the iOS build with #if !defined(TARGET_OS_IPHONE) || !TARGET_OS_IPHONE so the tests don't exist in the iOS binary at all. macOS, Linux and Windows still build and run them (guard is true there; on non-Apple TARGET_OS_IPHONE is undefined -> included). Files: tests/functests/MultipleLogManagersTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… logs (microsoft#1493) * Low-severity hardening: CSPRNG UUIDs (POSIX) + null-safe zlib error logs Two low-severity issues found during a repo-wide review: 1) PAL::generateUuidString POSIX/Android fallback built the UUID entirely from std::rand(), seeded once with srand(time(0) ^ nanos). std::rand() is a weak, predictable PRNG with a guessable time-based seed, so the session / event / instance identifiers derived from it were predictable. Source the bytes from std::random_device instead (backed by /dev/urandom on Linux/Android), matching the existing CorrelationVector.cpp / PseudoRandomGenerator usage. Windows (CoCreateGuid) and Apple (CFUUIDCreate) paths are unchanged. Test: PalTests.UuidGeneration extended to assert 1000 generated UUIDs are all distinct (in addition to the existing format/entropy checks). 2) zlib error-path logs passed stream.msg / zs.msg straight to a %s conversion. zlib leaves msg == Z_NULL for several error codes (Z_MEM_ERROR, Z_BUF_ERROR, after a failed deflateInit2), and printf("%s", NULL) is undefined behavior -- benign "(null)" on glibc but not guaranteed across the MSVC/Android/Apple CRTs this SDK targets. Guard with `msg ? msg : "(null)"` in ZlibUtils.cpp and the two HttpDeflateCompression.cpp sites. Verified on Linux host: UnitTests builds; PalTests (incl. the UUID uniqueness check) and ZlibUtilsTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review comment: avoid reopening entropy source per UUID lib/pal/PAL.cpp (generateUuidString, POSIX/Android path): std::random_device was default-constructed on every call, which reopens the entropy source (/dev/urandom) per UUID on the event-logging hot path. Mark it thread_local so it is opened once per thread and reused; each operator() still draws fresh CSPRNG bytes, so the unpredictability property is unchanged and per-thread isolation keeps it lock-free. Verified the distinctness guard (PalTests.UuidGeneration, 1000 unique UUIDs) still passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot round-1 comments zlib error logs (ZlibUtils.cpp:51, HttpDeflateCompression.cpp:47,83): zlib return codes are signed and frequently negative (Z_DATA_ERROR=-3, etc.). Logging them with %u misrepresented the value and was a format/type mismatch; use %d for both the step and the code. PAL.cpp generateUuidString (POSIX/Android): reduce std::random_device reads from 11 to 4 (random_device::max() spans the full unsigned int range, so 4x32 bits fills the 128-bit GUID), cutting per-event-ID entropy-source reads on the hot path. Also soften the comment: random_device is non-deterministic/CSPRNG-backed on our target platforms but the standard does not guarantee the backing source universally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Name UUID test magic numbers as constexpr constants Address review feedback on PalTests UuidGeneration: replace the repeated literals 36 (canonical UUID string length) and 1000 (uniqueness-check batch size) with named constexpr constants UuidStringLength and UuidBatchSize so the test's intent is clear at each use site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Unify PalTests local constants on constexpr Convert the remaining size_t const constants (NumQueries, NumBuckets) in the PseudoRandomGenerator test to constexpr so all local constants in PalTests.cpp use the same form. NumBuckets is used as an array bound, so constexpr also documents that it must be a compile-time constant. No behavior or codegen change (const-integral literals already fold to immediates). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ft#1495) * Harden runtime task execution and offline-storage edge cases These are latent correctness/robustness fixes found during a repo-wide review. WorkerThread.cpp: wrap the queued-task invocation `(*item)()` in try/catch. A task runs arbitrary work (storage I/O, HTTP encode, user DebugEventListener callbacks); an exception escaping the loop unwinds out of the thread entry function and calls std::terminate, killing the host process. Contain it and log. TaskDispatcher_CAPI.cpp: same exception barrier around `(*m_task)()` in Task_CAPI::OnCallback(), which runs on the host's external dispatcher thread. capi.cpp (mat_open_core): on the EALREADY path (a guest instance with the same config is already open) set ctx->handle to the existing instance's handle before returning, so the caller is not left with an uninitialized/stale handle. OfflineStorageHandler.cpp (Flush): null-guard m_offlineStorageMemory->GetSize() read; the subsequent block already null-checks the pointer, so reading it first was inconsistent and a potential null deref. OfflineStorageHandler (StoreRecord): the per-instance RAM cache size limit was held in a function-local `static`, so the first LogManager's CFG_INT_RAM_QUEUE_SIZE leaked to every other LogManager instance. Compute it once per instance in Initialize() into a member (preserving the original "compute once" intent). Tests: - PalTests.WorkerThreadContainsThrowingTask: a task that throws std/non-std exceptions does not tear down the worker thread; follow-up tasks still run. - TaskDispatcherCAPITests.ExecuteCallbackThatThrowsIsContained: a throwing CAPI task callback does not propagate back into the host dispatcher thread. Files changed: - lib/pal/WorkerThread.cpp - lib/pal/TaskDispatcher_CAPI.cpp - lib/api/capi.cpp - lib/offline/OfflineStorageHandler.cpp - lib/offline/OfflineStorageHandler.hpp - tests/unittests/PalTests.cpp - tests/unittests/TaskDispatcherCAPITests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot round-1 comments capi.cpp (mat_open_core, EALREADY path): also set ctx->result to match the returned status. The success path sets ctx->result before returning; the EALREADY early-return set ctx->handle but left ctx->result stale, inconsistent with the other entrypoints. TaskDispatcher_CAPI.cpp (Task_CAPI::OnCallback): log the contained exception instead of swallowing it silently, mirroring WorkerThread, so host apps/SDK logs can diagnose why a queued task failed. Added <exception> include for std::exception. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot round-2 comments WorkerThread.cpp: add explicit #include <exception>; the round-1 catch block uses std::exception but the TU only relied on transitive includes. capi.cpp (mat_open_core): the early error returns (invalid config, and the two HttpClient/TaskDispatcher creation catch blocks) left ctx->result and ctx->handle untouched. Since the public mat.h inline helpers return ctx.handle after calling in, callers could observe a stale/uninitialized handle on error. Set ctx->result = EFAULT and ctx->handle = 0 on all three paths (ctx is guaranteed non-null by mat_open / mat_open_with_params). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot round-3 comments capi.cpp (mat_open_core): if HttpClient_CAPI or TaskDispatcher_CAPI construction threw, clients[code] had already been populated (config + ctx_data assigned), but the catch returned EFAULT without removing it. That left an orphaned, half- initialized entry in the global clients map, so a later open with the same config would match ctx_data and wrongly return EALREADY. Call remove_client(code) on both creation-failure paths before returning EFAULT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ft#1501) * Make public headers safe for consumers building with -Werror Consumers that embed the SDK and compile with -Wall -Wextra -Werror (e.g. ONNX Runtime / Foundry Local via add_subdirectory) were broken by warnings emitted from inside the SDK's public headers. Two complementary changes fix this: Primary (covers every consumer + every warning flag): mark mat's exported public include directory as SYSTEM. find_package consumers already treat an imported target's includes as system; SYSTEM extends that to add_subdirectory/FetchContent consumers, so their -Werror no longer promotes SDK-header warnings to errors. Verified: an -isystem consumer compiles clean under -Wall -Wextra -Wpedantic -Wshadow -Wconversion -Werror on gcc and clang. Defense in depth (also helps non-CMake consumers and NO_SYSTEM_FROM_IMPORTED): - UNREFERENCED_PARAMETER(...) expanded to nothing on gcc/clang, leaving the parameter unused; it now casts to void, eliminating 5 -Wunused-parameter warnings across NullObjects.hpp and LogManagerProvider.hpp. - assert(!"msg") triggered -Wstring-conversion on clang; switched to the canonical assert(false && "msg") in ISemanticContext.hpp (2 sites). Result: including the main public headers under -Wall -Wextra now yields 0 warnings (was 5). SDK's own top-level build verified unaffected (75 UNREFERENCED_PARAMETER call sites compile clean). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Scope SYSTEM to the public include dir only Address Copilot review comment on lib/CMakeLists.txt:358. The SYSTEM keyword is a per-call flag, so a single target_include_directories(mat SYSTEM PUBLIC ... PRIVATE ...) also marked the SDK-internal PRIVATE include dirs as system for mat's own build, which would suppress warnings in the SDK's own internal headers under -Werror. Split into two calls: SYSTEM PUBLIC for the exported public dir, and a separate non-SYSTEM PRIVATE call for internal dirs. Verified via CMake property query: INTERFACE_SYSTEM_INCLUDE_DIRECTORIES contains only the public dir; the private internal dirs return to their original (non-system) treatment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…roject (microsoft#1499) * Add BUILD_CURL_HTTP_CLIENT option to build Linux without curl/TLS On the CPP11/curl path (non-Apple, non-Windows), the built-in libcurl HTTP client was always compiled and curl was a hard find_package(CURL REQUIRED) dependency -- pulling in curl and a TLS backend (OpenSSL/mbedTLS) even for hosts that already have their own HTTP stack. Add option(BUILD_CURL_HTTP_CLIENT ON). When OFF, the curl block is skipped (no find_package(CURL), no link, no -DHAVE_MAT_CURL_HTTP_CLIENT) and the build instead defines -DMATSDK_NO_DEFAULT_HTTP_CLIENT. mat/config.h then undefines HAVE_MAT_DEFAULT_HTTP_CLIENT centrally (regardless of the config preset), which the SDK already handles end-to-end: HttpClientFactory and HttpClient_Curl.cpp compile out, and LogManagerImpl's existing !HAVE_MAT_DEFAULT_HTTP_CLIENT branch requires the host to supply an IHttpClient via CFG_MODULE_HTTP_CLIENT. Default ON keeps existing behavior unchanged. Apple/Windows are unaffected (they use native HTTP stacks and never enter the curl block). Validated on WSL x64-linux: with OFF, libmat has no curl symbols and a consumer links with no -lcurl/-lTLS (1.43 MB stripped, vs 4.39 MB with curl+mbedTLS and 10.65 MB with curl+OpenSSL). Files changed: - CMakeLists.txt: BUILD_CURL_HTTP_CLIENT option + gating - lib/include/mat/config.h: central HAVE_MAT_DEFAULT_HTTP_CLIENT opt-out Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add MATSDK_MINIMAL_SQLITE: private feature-stripped SQLite to cut footprint The SDK uses SQLite only for its offline event-storage cache (plain tables, transactions, WAL, autovacuum/VACUUM, a few PRAGMAs, and one custom UTF-8 function), so most SQLite subsystems are dead weight. Add an option to compile a private SQLite from the vendored amalgamation with a set of amalgamation-safe strip flags (single source of truth: MATSDK_SQLITE_MINIMAL_DEFS), removing the external sqlite3 dependency and shrinking SQLite ~10.2% (.text) / ~12.5% (object). - Root CMakeLists.txt: add option(MATSDK_MINIMAL_SQLITE) (default OFF). In vcpkg mode, skip find_package(unofficial-sqlite3) when bundling, and emit a clear FATAL_ERROR pointing at the system-sqlite/minimal-sqlite features when neither provides SQLite (e.g. a bare [core] install). - lib/CMakeLists.txt: define MATSDK_SQLITE_MINIMAL_DEFS, compute MATSDK_BUNDLE_SQLITE (minimal OR vendored-Android), and build a single sqlite3_bundled. The strip flags are applied ONLY when MATSDK_MINIMAL_SQLITE is ON, so the default Android legacy build keeps its existing unstripped bundled SQLite. Warnings are disabled on the vendored target (/w on MSVC, -w on GCC/Clang for the stripped build) so the SDK's -Werror/-WX does not fire on amalgamation code. A static mat propagates the PRIVATE sqlite3_bundled through its link interface, so export+install it. - MSTelemetryConfig.cmake.in: skip find_dependency(unofficial-sqlite3) when bundled. - vcpkg port: add a minimal-sqlite feature (-DMATSDK_MINIMAL_SQLITE=ON) and move sqlite3 into a default system-sqlite feature so [core,minimal-sqlite] drops it. - docs/building-with-vcpkg.md: document the feature, the size win, and the static-absorption symbol-visibility caveat. SQLITE_OMIT_AUTOINIT and SQLITE_DEFAULT_MEMSTATUS=0 are deliberately NOT stripped: the former because skipSqliteInitAndShutdown lets the host skip the SDK's explicit sqlite3_initialize() (which the host cannot do against a private SQLite), the latter because the SDK arms a soft heap limit via sqlite3_soft_heap_limit64() that is only enforced while memory statistics are enabled. Validated: vendored Linux Debug (77 offline-storage/SQLite unit tests pass on the debug amalgamation), vcpkg [core,minimal-sqlite] consumer (links MSTelemetry::sqlite3_bundled, runs 10/10, external sqlite3 dropped), default vcpkg path regression (system-sqlite intact), and MSVC compile/link of sqlite3_bundled+mat. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot review: scope bundled-SQLite export to static mat - lib/CMakeLists.txt: only add sqlite3_bundled to the install/export set when mat is a STATIC_LIBRARY. A shared mat absorbs the private SQLite into libmat and does not propagate the PRIVATE dependency, so exporting the separate archive there was unnecessary and could let a consumer link a second SQLite copy. For a static mat the archive must stay exported because the static library propagates its PRIVATE dependency through its link interface (\$<LINK_ONLY:...>). - CMakeLists.txt: make the vcpkg dependency-mode status message reflect whether the external sqlite3 package or the private minimal SQLite is used. Verified with an isolated CMake export test: static mat exports m+sq (consumer linking only the namespaced lib resolves sq); shared mat exports only m and install(EXPORT) succeeds with sq excluded. Re-ran the vcpkg [core,minimal-sqlite] consumer (static x64-linux): 10/10. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify sqlite3_bundled PRIVATE-link comment (Copilot review) Correct the inline comment: a PRIVATE link of the bundled SQLite suppresses propagation of its include dirs / compile definitions, but a static mat still propagates the archive for linking via \$<LINK_ONLY:...> (hence it is exported for static builds); a shared mat absorbs it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg port: selectable TLS backend (curl-openssl/curl-mbedtls) + no-default-http-client feature Make the HTTP-client footprint a consumer choice instead of hardcoding curl[openssl]: - vcpkg.json: replace the base curl[openssl] dependency with three features -- curl-openssl (default; libcurl + OpenSSL), curl-mbedtls (libcurl + mbedTLS), and no-default-http-client (omit the built-in client). curl-openssl is a default feature so a plain install keeps current behavior; [core,no-default-http-client] drops curl entirely. - portfile.cmake: map the no-default-http-client feature to -DBUILD_CURL_HTTP_CLIENT=OFF via INVERTED_FEATURES. - CMakeLists.txt: when the built-in client is enabled in vcpkg mode but libcurl is not found, emit a clear FATAL_ERROR pointing at the curl-openssl/curl-mbedtls/ no-default-http-client features (instead of a bare find_package failure). - docs: document the size ladder (OpenSSL ~10.6MB / mbedTLS ~4.4MB / no-curl ~1.4MB) and the exact mbedTLS recipe -- crucially, the consumer must ALSO list curl with default-features:false at the top level, because vcpkg only honors curl's default-features:false for top-level dependencies (otherwise curl's ssl default pulls OpenSSL in transitively alongside mbedTLS). Validated on WSL with vcpkg: default resolves curl[openssl]+sqlite3; the documented mbedTLS recipe builds with mbedTLS only (no libssl/libcrypto, libcurl carries no OpenSSL symbols) and the consumer runs; [core,no-default-http-client] drops curl from the graph; [core,minimal-sqlite,no-default-http-client] drops curl and the external sqlite3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden HTTP-client features: CURL CONFIG mode + mutual-exclusivity guard (Copilot review) - CMakeLists.txt: in vcpkg mode use find_package(CURL CONFIG QUIET) and gate on TARGET CURL::libcurl. Forcing CONFIG selects the vcpkg-provided CURLConfig (which defines the imported target) rather than the module FindCURL, which on some CMake versions does not define CURL::libcurl and would fail at link. - portfile.cmake: fail fast when more than one of curl-openssl/curl-mbedtls/ no-default-http-client is selected. vcpkg cannot express mutual exclusivity, so a consumer requesting e.g. curl-mbedtls without [core] keeps the default curl-openssl and would union both TLS backends; the guard now errors with guidance to use the [core,...] form. Validated: the guard passes single selections and fires on curl-openssl+curl-mbedtls and curl-openssl+no-default-http-client; the default (curl-openssl) vcpkg consumer still configures via CURL CONFIG, links, and runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove the no-curl (no-default-http-client) option Drop the ability to build without the built-in libcurl HTTP client. That option only benefited consumers that already ship their own IHttpClient; the SDK's named consumers (and Apple/Windows, which use NSURLSession/WinInet) never needed it, and it added a fragile feature plus a config-flow opt-out. The TLS-backend selection (curl-openssl default / curl-mbedtls) and minimal-SQLite remain. - CMakeLists.txt: remove option(BUILD_CURL_HTTP_CLIENT) and the no-curl else branch; the curl HTTP client is always built on the CPP11/curl path again (keeping the find_package(CURL CONFIG) + TARGET CURL::libcurl hardening). - lib/include/mat/config.h: remove the MATSDK_NO_DEFAULT_HTTP_CLIENT -> HAVE_MAT_DEFAULT_HTTP_CLIENT opt-out. - vcpkg.json: remove the no-default-http-client feature. - portfile.cmake: remove the INVERTED_FEATURES mapping; the mutual-exclusivity guard now covers just curl-openssl vs curl-mbedtls. - docs: drop the no-curl row/section; note the size figures are worst-case (without consumer-side --gc-sections). Validated: vcpkg.json parses, CMake configures cleanly, and the mat target builds and links with the curl client compiled in. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg: use Apple's system libsqlite3/libz on macOS/iOS instead of vcpkg packages macOS/iOS ship libsqlite3 and libz as system libraries, so pulling and statically linking the vcpkg sqlite3 + zlib packages added ~1 MB of redundant code to Apple binaries. Link the system libraries instead -- consistent with the SDK's own Swift Package (which links .linkedLibrary("sqlite3"/"z")) and with how analogous telemetry SDKs (e.g. sentry-native) gate these deps off Apple platforms. - vcpkg.json: gate the zlib dependency and the system-sqlite feature's sqlite3 dependency to "!osx & !ios" so they are not installed on Apple. - CMakeLists.txt: on APPLE in vcpkg mode, find_package(SQLite3)/find_package(ZLIB) (CMake's modules resolve to the OS libraries) and set MATSDK_APPLE_SYSTEM_DEPS. - lib/CMakeLists.txt: link SQLite::SQLite3 + ZLIB::ZLIB on Apple; never bundle a private SQLite on Apple (MATSDK_MINIMAL_SQLITE is a no-op there since the system lib is already smaller). - MSTelemetryConfig.cmake.in: re-find system SQLite3 on Apple, the vcpkg unofficial-sqlite3 elsewhere. - docs: note the Apple system-lib behavior. Validated: non-Apple paths unchanged -- Linux vendored mat builds, and the Linux vcpkg consumer's generated config resolves unofficial-sqlite3 (if(OFF)) and runs. The Apple build itself needs validation on macOS/iOS CI (no Mac available here); the risk is whether find_package(SQLite3) resolves the system lib under the vcpkg Apple triplets' find-root settings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify mbedTLS guidance: [core,...] drops system-sqlite too The curl-openssl/curl-mbedtls guidance in the port's fatal-error messages and docs recommended cpp-client-telemetry[core,curl-mbedtls], but the [core,...] form (default-features:false) drops ALL default features -- including system-sqlite -- not just curl-openssl. That example yields a config-time failure with no SQLite backend selected. Update both FATAL_ERROR messages (portfile.cmake mutual-exclusivity guard, CMakeLists.txt libcurl-not-found) and the docs prose to show a complete, working feature set ([core,curl-mbedtls,system-sqlite]) and to note that [core,...] also drops system-sqlite, so a SQLite backend must be re-selected. Files: tools/ports/cpp-client-telemetry/portfile.cmake, CMakeLists.txt, docs/building-with-vcpkg.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix misleading curl 'optional' label and mbedTLS feature description Two doc/manifest accuracy fixes from Copilot review: - The dependency table labeled libcurl 'optional' for non-Windows/non-Apple, but since the no-curl option was removed, Linux/Android vcpkg builds always require curl (only the TLS backend is selectable). Relabel as required. - The curl-mbedtls feature description recommended [core,curl-mbedtls], which drops all defaults (incl. system-sqlite); note that a SQLite backend must be re-selected to avoid a configure-time failure. Files: docs/building-with-vcpkg.md, tools/ports/cpp-client-telemetry/vcpkg.json Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden [core,...] guidance and fix exported CURL find_dependency mode Five fixes from the Copilot review on the curl/SQLite feature interactions (all stem from vcpkg's [core,...] form dropping ALL default features, not just one): - portfile.cmake: fail fast on Linux/Android when no curl TLS backend is selected (verified: a real vcpkg install of [core,minimal-sqlite] now stops at the portfile with a complete [core,curl-openssl,system-sqlite] example, instead of a later, opaque libcurl-not-found error). - CMakeLists.txt libcurl message: show how to re-select a curl backend (not only mbedTLS) under [core,...], alongside a SQLite backend. - CMakeLists.txt SQLite message: include the valid [core,system-sqlite] path, not only [core,minimal-sqlite]. - MSTelemetryConfig.cmake.in: find_dependency(CURL CONFIG) so the exported package config uses the vcpkg CURLConfig that defines CURL::libcurl (the target MSTelemetryTargets references), matching the unofficial-sqlite3/ nlohmann_json CONFIG siblings and the root CMakeLists CURL CONFIG lookup. - docs: minimal-sqlite manifest example re-selects curl-openssl so the Linux/Android manifest actually configures. Files: CMakeLists.txt, cmake/MSTelemetryConfig.cmake.in, docs/building-with-vcpkg.md, tools/ports/cpp-client-telemetry/portfile.cmake Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Scope curl TLS-backend guards to Linux/Android only The mutual-exclusivity check (curl-openssl vs curl-mbedtls) previously ran on all platforms. Since curl-openssl is a default feature and the curl dependency is platform-filtered to linux|android, a cross-platform manifest that enables curl-mbedtls without [core] would falsely fail the port on Windows/macOS/iOS -- where curl is not used (WinInet / Apple HTTP) and neither feature pulls curl. Wrap both the mutual-exclusivity (count>1) and no-curl (count==0) checks in a single VCPKG_TARGET_IS_LINUX/ANDROID block so they only fire where the curl backend selection is actually meaningful. Verified on x64-linux: [core,minimal- sqlite] still fails with the no-curl message, and [curl-mbedtls] (no core) still fails with the mutual-exclusivity message. Files: tools/ports/cpp-client-telemetry/portfile.cmake Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg port tests: build the working tree, not a pinned release The port tests built the SDK from the portfile's pinned vcpkg_from_github REF (v3.10.161.1), so they never exercised the PR's own source -- and the macOS/iOS jobs failed because this PR's manifest drops the Apple sqlite3/zlib packages while the old pinned source still calls find_package(unofficial-sqlite3) unconditionally (the Apple system-libs branch only exists in the PR source). Add an opt-in MATSDK_VCPKG_SOURCE_DIR hook to portfile.cmake: when set, the port builds that local source; when unset (production installs), the pinned release is downloaded as before, so the published port behavior is unchanged. The five tests/vcpkg/* scripts set it to the repo root so the port tests validate the actual source + manifest together. Verified on Linux (x64-linux): the port now builds the working-tree SDK and the consumer passes 10/10; the macOS/iOS jobs will exercise the Apple system-libs branch (find_package(SQLite3)/ZLIB) instead of the dropped vcpkg packages. Files: tools/ports/cpp-client-telemetry/portfile.cmake, tests/vcpkg/test-vcpkg-{linux,macos,ios,android}.sh, tests/vcpkg/test-vcpkg-windows.ps1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix Windows vcpkg test to actually build the working tree On Windows, vcpkg runs portfiles in a sanitized environment and strips custom variables unless allow-listed via VCPKG_KEEP_ENV_VARS. Without it the portfile never saw MATSDK_VCPKG_SOURCE_DIR and silently fell back to the pinned release (v3.10.161.1), so the Windows port test validated the old release instead of the PR source while still reporting PASS. Allow-list the variable so the test builds the working tree, matching the Linux/macOS scripts (POSIX vcpkg passes the variable through, so they need no change). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Gate test suites on top-level project (default OFF for consumers) BUILD_UNIT_TESTS/BUILD_FUNC_TESTS defaulted to ON unconditionally, so a downstream project consuming this repo via add_subdirectory()/FetchContent built the whole test suite and required the third_party/googletest submodule. Default them ON only when this repo is the top-level project (PROJECT_IS_TOP_LEVEL on CMake >= 3.21, source-dir comparison on older CMake) and OFF when consumed as a subproject. Direct/CI builds are unchanged (top-level => ON) since build scripts rely on the default; verified BUILD_UNIT_TESTS/BUILD_FUNC_TESTS=ON for a top-level configure and OFF via add_subdirectory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot review: validate source dir, append VCPKG_KEEP_ENV_VARS - portfile.cmake: validate MATSDK_VCPKG_SOURCE_DIR points at a real checkout (CMakeLists.txt present) and fail early with a clear message instead of a confusing downstream CMake error. - test-vcpkg-windows.ps1: append MATSDK_VCPKG_SOURCE_DIR to VCPKG_KEEP_ENV_VARS instead of overwriting it, preserving any entries the caller/CI already set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Support consuming SDK as a CMake subproject in legacy mode Applies the two changes the ONNX Runtime consumer patch carried so they can be dropped from the downstream patch set. Change 1 (CMakeLists.txt): use CMAKE_CURRENT_SOURCE_DIR instead of CMAKE_SOURCE_DIR for the vendored sqlite/zlib/nlohmann include path, so the headers still resolve when the SDK is added via add_subdirectory/FetchContent (where CMAKE_SOURCE_DIR points at the consumer's root, not this repo). Change 2 (lib/CMakeLists.txt): extend the Android bundled-deps legacy path to also cover iOS. A cross-compile cannot reliably find a system libsqlite3, and the vendored zlib renames its exports to act_z_* (zlib/names.h) so a system libz cannot satisfy those symbols. iOS now builds the vendored sqlite amalgamation + bundled zlib, matching Android. Only affects legacy mode (MATSDK_USE_VCPKG_DEPS=OFF); the vcpkg Apple path still links system libs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Explicitly disable warning-as-error for the vendored SQLite TU on MSVC Address Copilot review comment on lib/CMakeLists.txt:470. The comment claimed the build drops /WX for the vendored SQLite translation unit, but the code only added /w. /w disables all warnings, but MSVC can still promote a non-suppressible warning to an error under an inherited /WX. Add /WX- so the code literally matches the comment's stated intent and cannot be broken by such a warning. Verified cl.exe accepts /w /WX- together. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify MATSDK_MINIMAL_SQLITE-on-Apple comment for the iOS legacy path Address Copilot review comment on lib/CMakeLists.txt:446. Adding iOS to the legacy bundled-SQLite path means MATSDK_MINIMAL_SQLITE is no longer a strict no-op on all Apple builds: iOS in legacy mode (MATSDK_USE_VCPKG_DEPS=OFF) bundles the amalgamation and applies the strip definitions to it, matching Android legacy. Clarify the comment so it no longer reads as a blanket 'no effect on Apple' statement. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Link system sqlite3 + zlib on Apple legacy builds (match repo convention) Take further inspiration from the ONNX Runtime consumer patch, verified against what the repo already does on Apple. The SDK's own iOS Xcode projects link libsqlite3.tbd + libz.tbd from the SDKROOT, Package.swift links .linkedLibrary("sqlite3")/("z"), and microsoft#1499 already links the system libsqlite3/libz on the vcpkg Apple path. Bundling is an Android-only convention (the NDK ships no system zlib). So the earlier change that made iOS legacy bundle sqlite+zlib was the inconsistent one; this aligns iOS with the rest of the repo. - Apple legacy (macOS + iOS) now links system `sqlite3 z` by portable names in a single elseif(APPLE) branch. macOS moves off find_package(ZLIB) + hardcoded Homebrew .a paths (non-relocatable) onto the same portable link names, so exported static packages stay relocatable. iOS no longer bundles. - iOS dropped from the MATSDK_BUNDLE_SQLITE gating and the bundled-zlib branch, which are now Android-only. - Exclude iOS from include_directories(/usr/local/include): that host (macOS) path must not be injected into an iOS cross-compile's search path where it can shadow the iOS SDK's own headers. - Linux legacy simplified to find_package(SQLite3) (the Homebrew .a fallbacks were macOS-only and are now handled by the Apple branch). Verified: Linux top-level and add_subdirectory legacy builds both produce libmat.so. The Apple legacy path is exercised by the macOS-latest CI leg (build-posix-latest, legacy mode); iOS cannot be built on this host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Drop redundant ZLIB include dir on the Linux legacy path target_include_directories(mat PRIVATE ${ZLIB_INCLUDE_DIRS}) was redundant: mat already links ZLIB::ZLIB (and SQLite::SQLite3), imported targets that propagate their own include directories. Verified: Linux legacy mat build still resolves <zlib.h> and produces libmat.so. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * vcpkg port: bump pinned REF to v3.10.173.1 to match the SDK version The port pinned v3.10.161.1 while the SDK source on this branch is at v3.10.173.1 (Version.hpp), leaving production installs two releases behind. Bump the portfile REF + SHA512 and the vcpkg.json version to v3.10.173.1; the SHA512 is computed from the release source tarball. Note: the port's minimal-sqlite and Apple system-sqlite features depend on CMake changes introduced by this PR that are not yet in any release tag. The in-repo port tests exercise them against local source via MATSDK_VCPKG_SOURCE_DIR, and the pinned REF must be advanced again to the release that includes these changes once it is cut. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Validate vcpkg release bump production port path After updating the vcpkg port REF/SHA512/version for a new SDK release, exercise the real production port path with MATSDK_VCPKG_SOURCE_DIR unset. This catches mismatches where the port manifest assumes source changes that are not present in the release tag the port downloads. When the new footprint features are present, validate the opt-in minimal-sqlite + curl-openssl feature set so release automation covers both release pinning and feature wiring before opening the vcpkg PR. Validation: - Parsed .github/workflows/vcpkg-release-bump.yml with PyYAML. - Verified the feature-selection expression resolves to cpp-client-telemetry[core,minimal-sqlite,curl-openssl] for the current port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove vcpkg release-bump production validation Remove the release-bump production-port validation added in 93dd36e. The vcpkg port update will instead rely on the explicit release sequencing: merge the SDK source changes, cut a new SDK tag, then bump the vcpkg REF, SHA512, and version to that tag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Revert "Remove vcpkg release-bump production validation" This reverts commit dd6007a. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…der (microsoft#1512) * Replace malformed UTF-8 instead of throwing in PayloadDecoder * Address review: reword comment and add PayloadDecoder regression tests - Reword the DecodeRequest comment to state the local rationale for error_handler_t::replace instead of referencing the HAVE_MAT_AI-gated AIJsonSerializer, which is not part of the default OSS build. - Add PayloadDecoderTests covering the invalid-UTF-8 regression: a record whose string field carries a non-UTF-8 byte must be decoded without throwing type_error.316, and the bad byte must surface as U+FFFD. Also verifies valid UTF-8 is left untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ab3a16b-98f7-4fa7-a933-684b34196436 * Harden PayloadDecoder tests against -Werror char conversion Address review feedback on the regression tests: - Inject the invalid UTF-8 byte via push_back(static_cast<char>(0xFF)) and search for it with static_cast<char>(0xFF) instead of a '\xFF' string/char literal, which relies on implementation-defined char conversion and can trip -Werror constant-conversion on some toolchains. - Drop the product-specific phrasing from the test comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ab3a16b-98f7-4fa7-a933-684b34196436 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ab3a16b-98f7-4fa7-a933-684b34196436
…icrosoft#1509) Broken normalization in the lib/modules subrepo is causing enlistment issues in the Edge project; rolling forward the submodule to the commit 7bd8b516e2d93d1704834e0895733ae7bc2d1f43 picks up fixes made in that repo that amend this. Co-authored-by: bmehta001 <bmehta001@users.noreply.github.com>
* Add Android HTTP transport selection for vcpkg Decouple Android HTTP transport choice from MATSDK_USE_VCPKG_DEPS so consumers can use vcpkg-provided native dependencies while still packaging the Java/JNI transport. Files changed: - CMakeLists.txt, lib/CMakeLists.txt, cmake/MSTelemetryConfig.cmake.in: add MATSDK_ANDROID_HTTP_CLIENT and export Android Java source metadata. - tools/ports/cpp-client-telemetry/*, docs/building-with-vcpkg.md: add android-java-http feature, source guard, and consumer documentation. - lib/pal/posix/*_Android.cpp: align initializer order for current NDK Clang builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Default Android vcpkg builds to Java HTTP Make Android AUTO select the Java/JNI HTTP transport regardless of dependency sourcing, and require explicit android-curl-* features for native curl. Files changed: - CMakeLists.txt: resolve Android AUTO to JAVA. - tools/ports/cpp-client-telemetry/portfile.cmake: use the in-repo checkout for overlay builds and add explicit Android curl feature handling. - tools/ports/cpp-client-telemetry/vcpkg.json: move Android curl to explicit android-curl-openssl/android-curl-mbedtls features. - docs/building-with-vcpkg.md: document Java default and curl escape hatch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Address Copilot review for Android transport PR NetworkInformationImpl_Android.cpp: initialize m_registeredCount so callback registration bookkeeping does not read an indeterminate value. Verified m_registeredCount is incremented/decremented in lib/pal/NetworkInformationImpl.hpp. DeviceInformationImpl_Android.cpp: initialize m_registeredCount for the same callback bookkeeping path. Verified m_registeredCount is incremented/decremented in lib/pal/DeviceInformationImpl.hpp. MSTelemetryConfig.cmake.in: update the curl dependency comment to reflect Linux, explicit Android curl builds, and macOS-without-Apple-HTTP only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Address Copilot follow-up review for Android transport portfile.cmake: make in-repo overlay source detection require SDK-specific paths so registry/vcpkg checkouts cannot be mistaken for cpp_client_telemetry. MSTelemetryConfig.cmake.in: set MSTelemetry_ANDROID_JAVA_SOURCE_DIR only for Java-transport packages; set it empty otherwise. Android PAL: initialize m_type and m_os_architecture alongside the callback counters so inline getters never read indeterminate values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Address final Copilot comments on Android transport MSTelemetryConfig.cmake.in: export NONE instead of an empty Android transport sentinel for non-Android packages, while keeping the Java source directory empty outside JAVA transport. portfile.cmake: add the [core,...]/default-features=false remediation hint to the curl backend mutual-exclusivity error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 --------- Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41
…crosoft#1506) * Create the offline event cache with owner-only (0600) permissions The SQLite offline cache buffers pending telemetry events (tenant ids, user identifiers, serialized event payloads) but was created with SQLite's default file permissions (0644 -- world-readable), letting any co-located user on a POSIX system read the buffered event stream or tamper with pending events. Restrict the database file to 0600 immediately after opening it in SQLiteWrapper::open. This runs before WAL is enabled, so the -wal/-journal companion files inherit 0600 from the main database file (SQLite's findCreateFileMode derives their mode from the main db). The chmod is best-effort (a failure, e.g. an in-memory ":memory:" database, does not fail the open) and POSIX-only -- on Windows the Unix mode bits are meaningless (NTFS ACLs govern access). Adds a POSIX unit test asserting the cache and its companions are not group/world accessible. Verified on Linux (umask 022): a bare SQLite db is created 0644; with this change the cache and its -wal/-journal are 0600. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Also tighten pre-existing companion files on open (round 1) Address Copilot review: only the main database was chmod'd, so a cache created by an older SDK (or a companion left behind after a crash) could retain the old world-readable 0644 mode on its -wal/-shm/-journal files. Best-effort chmod any pre-existing companions to 0600 on open, ignoring ENOENT. Add ExistingFilesAreTightenedOnOpen covering the migration path (loosen db + plant a leftover -wal, then verify reopen re-tightens both). Files: - lib/offline/SQLiteWrapper.hpp - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Silence in-memory chmod warning and clean up test companions (round 2) Address Copilot review round 2: - The main-database chmod now ignores ENOENT, so opening an in-memory (":memory:") database -- which LogManagerImpl uses when no tenant token is configured -- no longer logs a spurious permission-tightening warning on every open (there is no file to secure). - ExistingFilesAreTightenedOnOpen now removes the database and its companion files (including the planted -wal) at the end, so they don't leak into other tests that reuse the same storage filename. Files: - lib/offline/SQLiteWrapper.hpp - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove WAL companion files in shared test teardown (round 3) Address Copilot review round 3: CacheFileCreatedOwnerReadWriteOnly (and any other WAL-mode test) relied on shutdownAndRemoveFile(), which only removed the main database and could leave -wal/-shm/-journal companions behind to pollute the temp dir. Remove the companions in the shared teardown helper so every test is covered, and drop the now-redundant explicit cleanup from ExistingFilesAreTightenedOnOpen. Files: - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…icrosoft#1507) * Cap curl HTTP response body size to prevent memory-amplification DoS The libcurl transport buffered the entire collector response with no upper bound: WriteMemoryCallback and WriteVectorCallback grew (realloc / vector insert) until the whole body was received. A hostile or MITM'd collector could return an arbitrarily large body and drive unbounded memory growth in the embedding process. OneCollector responses are tiny (status, kill-switch tokens, retry-after, small config), so a generous fixed cap never rejects a legitimate response. Add a 16 MB cap (kMaxResponseBytes) enforced overflow-safely in both curl write callbacks; exceeding it returns a short count so curl aborts the transfer with CURLE_WRITE_ERROR (the upload becomes a network failure and is retried). Add HttpClientCurlResponseCapTests covering the oversized-abort and large-under-cap paths. Files: - lib/http/HttpClient_Curl.hpp - tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Guard response cap against overflow and fix test request lifetime (round 1) Address Copilot review on the response-size cap: - Guard the size * nmemb product in both curl write callbacks against size_t multiplication overflow before using it, so the "overflow-safe" cap check cannot operate on a wrapped length. - The response-cap test fixture now owns the IHttpRequest (the client only stores a raw pointer and never frees it) and releases it in TearDown on the main thread, fixing the per-test leak. Freeing it in OnHttpResponse would destroy the CurlHttpOperation from within its own async task (whose destructor waits on that task -- a self-join deadlock), so teardown-time release is used instead. sendAndWait() also resets result state up front so the helper is safe to reuse. Files: - lib/http/HttpClient_Curl.hpp - tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Synchronize test response-size field to avoid a data race (round 2) Address Copilot review round 2: m_responseBodySize was written by the test thread in sendAndWait() and read by the HttpServer reactor thread in onHttpRequest() without synchronization -- a data race that would trip TSAN. Write it under the existing mutex and read it under the same lock (into a local) to establish a happens-before edge. Files: - tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rosoft#1505) String.prototype.trim() (added in microsoft#1455 to complete the newline sanitization) is not implemented by the Windows Script Host JScript engine that runs version.js via cscript, so every Windows CI build logged 'version.js(48,3) Microsoft JScript runtime error: Object doesn't support this property or method'. Replace trim() with an ES3-compatible global-anchored regex that strips leading/trailing whitespace (including all trailing newlines), so the script runs cleanly while keeping the complete-sanitization behavior CodeQL asked for. Verified: 'cscript //nologo version.js' now exits 0 and regenerates Version.hpp; confirmed the JScript engine rejects .trim() with the same error seen in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ts (microsoft#1508) * Cap HTTP response body size across WinInet, WinRt, and Apple transports Extends the memory-amplification DoS hardening (a hostile or MITM'd collector returning an oversized body to exhaust process memory) beyond the libcurl transport to the remaining platform transports. Introduces a single shared constant MAX_HTTP_RESPONSE_SIZE (16 MB) in IHttpClient.hpp so every transport uses the same generous ceiling, well above any legitimate OneCollector or config response. - WinInet: bound m_bodyBuffer in the InternetReadFile loop; over-cap aborts the read and the request is reported as a failure (retried). - WinRt: reject a ReadAsBufferAsync buffer whose length exceeds the cap without copying it; report NetworkFailure. - Apple (NSURLSession): reject a completion-handler NSData larger than the cap without copying it; report NetworkFailure. The libcurl transport is capped separately in its own focused change; a later cleanup can unify its constant onto MAX_HTTP_RESPONSE_SIZE. Files: - lib/include/public/IHttpClient.hpp - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Map WinInet oversize to NetworkFailure and guard Apple nil data (round 1) Address Copilot review: - WinInet: the oversize-response abort set ERROR_NOT_ENOUGH_MEMORY, which fell through to the default case (LocalFailure). Use ERROR_HTTP_INVALID_SERVER_- RESPONSE so it maps to HttpResult_NetworkFailure, consistent with the WinRt and Apple transports (still retried, but correctly classified). - Apple: guard the success-path copy on a non-zero length and cast data.length to size_t, so a nil NSData (bytes == nullptr) never performs pointer arithmetic on nullptr (undefined behavior). Files: - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Stream response bodies to enforce the cap before full materialization (round 2) Address Copilot review round 2: the previous checks ran after the framework had already materialized the whole response body, so an oversized response could still drive a large allocation. Rework each transport to bound memory to the cap: - WinInet: check before every append (pre-loop and in-loop) so m_bodyBuffer never exceeds MAX_HTTP_RESPONSE_SIZE, not "cap + one chunk". (Validated: the Windows `mat` library compiles.) - WinRt: request with HttpCompletionOption::ResponseHeadersRead (so the body is not pre-buffered) and stream it via ReadAsInputStreamAsync in 64 KB chunks, aborting the moment the cap would be exceeded. - Apple: replace the completionHandler NSURLSession API (which materializes the full NSData) with a streaming NSURLSessionDataDelegate that accumulates in didReceiveData: and cancels the task once the cap would be exceeded; an over-cap transfer is surfaced as NetworkFailure (retried). The WinRt and Apple rewrites target UWP/macOS toolchains that aren't available locally, so they are review-verified and must be built/tested on-device before merge. Files: - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden streaming transports: guard WinRt read faults, fix Apple block cast (round 3) Address Copilot review round 3 on the streaming rework: - WinRt: concurrency::task::wait()/get() rethrow if ReadAsInputStreamAsync or a chunk ReadAsync faults (e.g., connection reset) even when the status looks completed. Wrap the whole streamed-read in try/catch so a fault maps to HttpResult_NetworkFailure instead of escaping onRequestComplete and crashing. - Apple: cast the dictionary value (stored as id) back to the concrete block type in didCompleteWithError: to avoid an incompatible-pointer-types warning (which fails builds under -Werror). Files: - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * WinRt: map cancel to Aborted and clear partial body on rejection (round 4) Address Copilot review round 4: HttpResponseDecoder processes any non-empty response body regardless of HttpResult (processBody runs when GetBody() is non-empty), so a partial body left on a rejected streamed response could be parsed for kill-switch/stats. In the WinRt streaming reader: - Map a caller-initiated cancellation (task_status::canceled, from cancel()) to HttpResult_Aborted instead of NetworkFailure. - Clear response->m_body on every non-success path (cancel, read failure, over-cap, and streaming exceptions) so no partial body is processed. (WinInet and Apple never attach a partial body to the response on rejection, so they need no change.) Files: - lib/http/HttpClient_WinRt.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix Apple response-cap delegate cleanup: NSMutableSet uses removeObject: The streaming session delegate's didCompleteWithError: cleanup called [_overCap removeObjectForKey:key], but _overCap is an NSMutableSet, which has no removeObjectForKey: selector (that belongs to NSMutableDictionary). This was a copy-paste from the _handlers/_buffers dictionary cleanup two lines above and fails to compile, breaking the entire Apple/macOS mat build. Use the correct NSMutableSet selector, removeObject:. Validation (macOS arm64, Apple HTTP transport): - libmat builds clean; full host UnitTests 518/518 pass. - End-to-end test against a local HttpServer through the real HttpClient_Apple: under-cap (64 KB) -> HttpResult_OK with full body; over-cap (16 MB + 1 MB) -> HttpResult_NetworkFailure with an empty body and no crash; exactly MAX_HTTP_RESPONSE_SIZE (16 MB) -> HttpResult_OK with full body. Stable over 8 repeats (no delegate state races). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t#1503) * Add a public-header gate CI job; make CompliantByDefaultFilterApi self-contained Consumers that embed the SDK (e.g. ONNX Runtime / Foundry Local) compile their own translation units -- which include our public headers -- under strict warning flags: -Wall -Wextra -Werror on GCC/Clang (plus -Wshorten-64-to-32 on Clang) and /W4 /WX on MSVC, suppressing third-party headers via -isystem / /external:W0. When that suppression is defeated (include order, PCH, or NO_SYSTEM_FROM_IMPORTED), any warning or missing include in our headers breaks the consumer build. This adds a CI gate that compiles every public header on its own, with no -isystem suppression, under those flags on GCC, Clang, and MSVC, so header issues surface here instead of at integration time: - tests/headers/check_public_headers.sh (GCC + Clang) - tests/headers/check_public_headers.cmd (MSVC) - .github/workflows/public-header-gate.yml The gate caught one real self-containment bug: CompliantByDefaultFilterApi.hpp uses std::vector<uint8_t> but did not include <cstdint>, so it only compiled when something else pulled <cstdint> in first. Added the include. VariantType.hpp is excluded: it is an implementation fragment included by Variant.hpp (which defines VariantMap/VariantArray and the needed std headers first), not a standalone header. Validated locally: all 40 public headers pass (39 compiled + VariantType excluded) on g++, clang++, and MSVC cl /W4 /WX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot round 1 on microsoft#1503: gate .h headers, tighten flags, fix MSVC includes - Gate both *.hpp and *.h so the flat C API headers (mat.h, CommonFields.h) are covered, not just *.hpp. Both compile clean standalone under the gate flags. - Drop -Wno-unused-but-set-variable from the GCC/Clang flags: no public header relies on it, so removing it makes the gate stricter. Kept -Wno-unused-parameter (mirrors this repo's WARN_FLAGS; intentional unused params use UNREFERENCED_PARAMETER). - MSVC gate: add /I lib\include so headers that conditionally pull mat/config.h (e.g. CsProtocol_types.hpp) exercise the same include graph as the GCC/Clang gate. Validated locally: 41/41 public headers pass on g++, clang++, and MSVC cl /W4 /WX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fold the public-header gate into existing CI workflows Instead of a standalone workflow, add the header gate as an isolated job in the workflows that already run on the same triggers: - build-posix-latest.yml gains a 'public-headers' job (GCC/Clang on ubuntu). - test-win-latest.yml gains a 'public-headers' job (MSVC on windows). Removes .github/workflows/public-header-gate.yml. The gate scripts under tests/headers/ are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Header gate: drop -Wno-unused-parameter so it mirrors real consumer flags The gate suppressed -Wunused-parameter, which is not part of the strict consumer flag set it claims to mirror (-Wall -Wextra -Werror). That hid a real break: NullObjects.hpp overrides left parameters unused because the old UNREFERENCED_PARAMETER macro expanded to nothing on GCC/Clang, so a consumer including LogManager.hpp with plain -Wall -Wextra -Werror failed to compile while the gate passed. With the macro now expanding to (void)(...) (merged from main), the headers are clean without the suppression; drop it so the gate actually catches this class of consumer break. Verified: 41/41 public headers pass standalone on g++ and clang++ without -Wno-unused-parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fail the public-header gate when no headers are found or PUB is missing The gate could silently pass without compiling anything: with nullglob the shell header globs expand to nothing (and the batch FOR loop runs zero times) when the computed public-header directory is wrong, so a miscomputed path reported success while testing nothing. Both scripts now validate the public-header directory exists and fail if zero headers were compiled. Also give the MSVC script a unique per-invocation work directory (%TEMP%\pubhdrgate_<rand>) so concurrent runs on the same machine cannot clobber each other's temporary translation unit, and clean it up on exit. Files: tests/headers/check_public_headers.sh, tests/headers/check_public_headers.cmd Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Strengthen public header gate coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use precise wall-clock time where available and retain nanosecond-derived 100 ns ticks on POSIX so record.time no longer truncates every event to milliseconds. Add regression coverage for POSIX timestamp precision.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 05d1030e-75b0-447f-9856-65091d59a97f
Resolve GetSystemTimePreciseAsFileTime once instead of repeating module and symbol lookups for every event timestamp.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 05d1030e-75b0-447f-9856-65091d59a97f
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Summary
GetSystemTimePreciseAsFileTimeon Windows when available, with a Windows 7-compatible fallbackFixes microsoft#1514.
Validation
git diff --checkpassedgtest/gtest.liband/WXfailures from pre-existingHAVE_MAT_AImacro redefinition / exception-mode warnings