Skip to content

Conversation

austinderek
Copy link

To make the code more readable and make Unmarshal use less memory:

goos: darwin
goarch: arm64
pkg: encoding/xml
cpu: Apple M4
│ old │ new │
│ sec/op │ sec/op vs base │
Unmarshal-10 3.818µ ± 1% 3.808µ ± 2% ~ (p=0.869 n=10)

         │     old      │                 new                 │
         │     B/op     │     B/op      vs base               │

Unmarshal-10 7.586Ki ± 0% 7.555Ki ± 0% -0.41% (p=0.000 n=10)

         │    old     │                new                │
         │ allocs/op  │ allocs/op   vs base               │

Unmarshal-10 185.0 ± 0% 184.0 ± 0% -0.54% (p=0.000 n=10)

Updates golang#62121


🔄 This is a mirror of upstream PR golang#75483

@austinderek austinderek force-pushed the master branch 30 times, most recently from 37c78b5 to 72ba117 Compare September 17, 2025 02:25
@austinderek austinderek force-pushed the master branch 27 times, most recently from fe3ba74 to d17a28a Compare September 29, 2025 16:04
@austinderek austinderek deleted the feat-xml-type-assert branch September 29, 2025 16:05
Copy link

staging bot commented Sep 29, 2025

HackerOne Code Security Review

🟢 Scan Complete: 2 Issue(s)
🟠 Validation Complete: One or more Issues looked potentially actionable, so this was escalated to our network of engineers for manual review. Once this is complete you'll see an update posted.

Here's how the code changes were interpreted and info about the tools used for scanning.

📖 Summary of Changes The modifications focus on XML encoding and decoding mechanisms in Go, specifically enhancing type assertion handling. Changes involve replacing direct type assertions with reflect.TypeAssert method calls across marshal and unmarshal functions. These updates aim to improve type checking and interface conversion processes in the XML marshaling and unmarshaling operations. | File | Summary | | --- | --- | | src/encoding/xml/marshal.go | The changes involve replacing type-specific method calls with reflect.TypeAssert for interface type checks in marshalValue, marshalAttr, marshalStruct, and related methods, improving type assertion handling in the XML marshaling process. | | src/encoding/xml/read.go | The changes involve replacing direct type assertions with reflect.TypeAssert method calls in several functions, particularly in unmarshalAttr and unmarshal methods. This modifies how type checking and interface conversion are performed, likely improving type safety and potentially performance. |
ℹ️ Issues Detected

NOTE: These may not require action!

Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem.

How will I know if something is a problem?
When validation completes, any concerns that warrant attention prior to merge will be posted as inline comments. These will show up in 2 ways:

  • Expert review (most cases): Issues will be posted by experts who manually reviewed and validated them. These are real HackerOne engineers (not bots) reviewing through an integrated IDE-like tool. You can communicate with them like any other reviewer. They'll stay assigned and get notified with commit & comment updates.
  • Automatically: In cases where our validation checks have highest confidence the problem is legitimate and urgent. These will include a description of contextual reasoning why & actionable next steps.
File & Line Issue
src/encoding/xml/marshal.go Line 448 The code changes replace the use of reflection type comparison (typ.Implements) with the new reflect.TypeAssert function. While this is generally a good change, there's a subtle issue in the implementation. The original code checked if a type implements an interface before attempting to use it, but the new code attempts the type assertion directly. If the type assertion fails, it will return a zero value and false, which is handled correctly. However, this approach could potentially lead to runtime panics if the value is nil and the code attempts to call methods on it. The original code would avoid this by checking if the type implements the interface first.
src/encoding/xml/read.go Line 261 The code changes replace type assertions using Interface().(Type) with the safer reflect.TypeAssert[Type] pattern. While this is generally an improvement, there's a subtle security issue: the new code checks for interface implementation differently. The original code first checked if the type implements an interface before performing the type assertion, but the new code does the type assertion directly. If there's any case where the original code's type check would fail but the new TypeAssert would succeed (or vice versa), this could lead to unexpected behavior when processing XML data, potentially causing security issues with malformed XML input.
🧰 Analysis tools - [ ✅ ] [HackerOne AI Code Analysis](https://www.pullrequest.com/blog/harnessing-ai-to-pinpoint-security-hotspots-in-code-review-a-deep-dive/) - [ ✅ ] [HackerOne AI Code Validation](https://www.hackerone.com/blog/ai-triage-code-validation-security) - [ ✅ ] [semgrep](https://semgrep.dev?&utm_source=hackerone&utm_campaign=pullrequest) - [ ✅ ] gosec - [ ✅ ] bandit

⏱️ Latest scan covered changes up to commit bb6bb30 (latest)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants