Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify invalid IBDO data/array modifications. #538

Merged

Conversation

jdcove2
Copy link
Collaborator

@jdcove2 jdcove2 commented Aug 17, 2023

This PR identifies invalid modifications to the IBDO data/array. This pertains only to IBDO data returned as a byte array since data returned as a ChannelFactory is immutable. An invalid modification is when a byte array is returned, the elements are modified and IBDO.setData(...)/IBDO.setChannelFactory(...) is NOT called. The problem occurs when the IBDO data is backed by a channel factory, a byte array is created and returned from the ChannelFactory, the byte array is modified and then the place competes with the byte array being garbage collected without the modifications being given back to the IBDO.

@jdcove2 jdcove2 force-pushed the IbdoArrayModification branch 3 times, most recently from 170619a to 27c789f Compare August 17, 2023 19:31
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approach looks good. Could use a little cleanup to improve readability and maintainability.

See commit 7e901af on the PR_538_extract_SafeUsageChecker branch for a suggested refactor to help isolate the safe usage checks.

@jdcove2 jdcove2 force-pushed the IbdoArrayModification branch from 27c789f to 95fc212 Compare August 22, 2023 21:57
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More cleanup recommendations. All are cosmetic, aimed at improving maintainability.

src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
src/test/java/emissary/core/BaseDataObjectTest.java Outdated Show resolved Hide resolved
@jdcove2 jdcove2 force-pushed the IbdoArrayModification branch from 95fc212 to 16dab83 Compare August 31, 2023 14:00
@jdcove2 jdcove2 requested a review from drivenflywheel August 31, 2023 14:06
drivenflywheel
drivenflywheel previously approved these changes Aug 31, 2023
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🚀

@smcgrattan smcgrattan self-requested a review September 1, 2023 16:29
smcgrattan
smcgrattan previously approved these changes Sep 1, 2023
@jpdahlke jpdahlke added this to the v8.0.0-M5 milestone Sep 5, 2023
@jpdahlke jpdahlke modified the milestones: v8.0.0-M5, v8.0.0-M6 Sep 15, 2023
@jpdahlke
Copy link
Collaborator

Should this be feature flagged and defaulted to off to begin integration?

@jpdahlke jpdahlke self-requested a review September 18, 2023 01:40
Copy link
Collaborator

@jpdahlke jpdahlke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awaiting response on feature-flag feedback.

@drivenflywheel
Copy link
Collaborator

Should this be feature flagged and defaulted to off to begin integration?

Should be relatively easy to add a SafeUsageChecker.cfg file with an ENABLED = "false" entry. That flag could no-op most SafeUsageChecker calls and force checkForUnsafeDataChanges(..) to return false.

drivenflywheel
drivenflywheel previously approved these changes Sep 18, 2023
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweaks suggested, notably correcting an assertion message ("true" vs "false")

src/test/java/emissary/core/SafeUsageCheckerTest.java Outdated Show resolved Hide resolved
src/main/java/emissary/core/SafeUsageChecker.java Outdated Show resolved Hide resolved
@jdcove2 jdcove2 force-pushed the IbdoArrayModification branch from 9c1e6e6 to 1ccf908 Compare September 20, 2023 19:25
Copy link
Collaborator

@smcgrattan smcgrattan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well done

@jpdahlke jpdahlke added the feature A new feature that does not exist today label Oct 2, 2023
@jpdahlke jpdahlke dismissed their stale review October 3, 2023 14:05

configurable update made

@jpdahlke jpdahlke merged commit 4790dd6 into NationalSecurityAgency:master Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature that does not exist today
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants