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

Address CVE-2024-22871 and cleanup older patterns #491

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

EthanEChristian
Copy link
Contributor

No description provided.

@EthanEChristian EthanEChristian self-assigned this Apr 6, 2024
Copy link
Collaborator

@mrrodriguez mrrodriguez left a comment

Choose a reason for hiding this comment

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

Looks good. I have only 1 smaller comment. The compiler changes are a bit to wrap my head around, but I think I followed along. Perhaps a bit of inline explanation there would be nice.

The removal of the use of an unnecessary atom for the compile-constraints is quite nice.

.gitignore Outdated
@@ -17,3 +17,6 @@ figwheel_server.log
*.iml
.clj-kondo
.lsp
node_modules/*
package.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

May not be a bad idea to check both the package*.json files into gh instead for stability. But won't block on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems reasonable:
4d0e1dc

Copy link

@svirl svirl left a comment

Choose a reason for hiding this comment

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

Hello from a first-time reviewer.
Liked the shadowing bit; one less atom is nice.

@EthanEChristian
Copy link
Contributor Author

Looks good. I have only 1 smaller comment. The compiler changes are a bit to wrap my head around, but I think I followed along. Perhaps a bit of inline explanation there would be nice.

The removal of the use of an unnecessary atom for the compile-constraints is quite nice.

I added some comments here:
fcfc4b1

I didn't think commenting on the changes to the TestNode record itself made sense documented inline, so i will cover them here.

Previously the testNode compilation was evaluating a structure like:

{:test (fn [] ... ) 
 :constraints (....)}

This was odd as the constraints didn't need to be evaluated from what i could see, and were actually adding to bloat in the generated classes, which in turn actually breaks the assertion here:
https://github.com/cerner/clara-rules/blob/3d3c776dd19f1fc80d50ae13c9d1fc51acc40da0/src/main/clojure/clara/rules/compiler.clj#L1971-L2000
in the sense that the additional evaluated hash-map was generating its own Static variables within the class files, meaning that the number of functions that could be eval'd in a batch was greatly diminished for some use-cases.

Copy link
Collaborator

@mrrodriguez mrrodriguez left a comment

Choose a reason for hiding this comment

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

Extra comments look good. Nothing blocking from me.

package.json Outdated
@@ -0,0 +1,5 @@
{
"dependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that important since we aren't publishing this for any production use, but I'd typically think it should be a "devDependencies" entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shows my lack of experience outside of the clojure side of things, updated here:
e0baa05

@EthanEChristian EthanEChristian merged commit 759ab40 into oracle-samples:main Apr 16, 2024
1 check passed
@EthanEChristian EthanEChristian deleted the cve-and-perf branch April 16, 2024 13:38
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.

3 participants