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.

Sign up for GitHub

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

Jump to bottom

Fix overflow conditions in prefetch plugin #8660

Merged
merged 3 commits into from
May 9, 2022
Merged

Fix overflow conditions in prefetch plugin #8660

merged 3 commits into from
May 9, 2022

Conversation

moonchen
Copy link
Contributor

@moonchen moonchen commented Feb 9, 2022 •
edited
Loading

In the prefetch plugin:

  1. Specify integer size limitations in README.
  2. Specify underflow/overflow behavior in README.
  3. Modify evaluate() to handle integer size and under/overflow conditions.
  4. Refactor and add unit tests.

This PR addresses #8569.

@masaori335
Copy link
Contributor

@ezelkow1 please add Mo (@moonchen) to the ci list too.

@masaori335 masaori335 added this to the 10.0.0 milestone Feb 9, 2022
@moonchen
Prefetch plugin: improve over/underflow handling
a37fd9d 
1. Specify integer size limitations.
2. Specify underflow/overflow behavior.
3. Refactor and add unit tests for evaluate().
@apache apache deleted a comment from ezelkow1 Mar 14, 2022
@apache apache deleted a comment from ezelkow1 Mar 14, 2022
@apache apache deleted a comment from vmamidi Mar 14, 2022
@apache apache deleted a comment from ezelkow1 Mar 14, 2022
@bryancall bryancall requested a review from ywkaras April 25, 2022 23:40
@ywkaras
Copy link
Contributor

ywkaras commented Apr 26, 2022

I and others are having trouble running the prefetch plugin ( #8216 ). @moonchen can you provide, as an example, a line from your remap.config file? I'd like to write an Au test case for prefetch and run it with a build including this change.

ywkaras
ywkaras reviewed Apr 26, 2022
View reviewed changes
plugins/prefetch/evaluate.h Outdated
#pragma once
#include "common.h"

String evaluate(const String &v);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing end-of-line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Added an eol.

ywkaras
ywkaras reviewed Apr 26, 2022
View reviewed changes
doc/admin-guide/plugins/prefetch.en.rst Outdated
subtracting integer numbers.
subtracting integer numbers. The operands will be treated as unsigned
32-bit integers. Invalid numbers are treated as zeroes, and numbers
too large will be interpreted as 2^32-1. If subtraction results in
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean 64 not 32?

Also have you tried https://restructuredtext.documatt.com/element/superscript.html ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose 32-bit operands and 64-bit results here so that addition doesn't ever overflow. That way there doesn't need to be a special rule for handling overflows when adding.

Thanks for pointing out the superscript element. I've changed the exponent to a superscript.

@moonchen
Copy link
Contributor Author

moonchen commented Apr 26, 2022 •
edited
Loading

I and others are having trouble running the prefetch plugin ( #8216 ). @moonchen can you provide, as an example, a line from your remap.config file? I'd like to write an Au test case for prefetch and run it with a build including this change.

I'm testing using these rules:

map http://example.com http://origin.com \
    @plugin=cachekey.so @pparam=--remove-all-params=true \
    @plugin=prefetch.so \
        @pparam=--front=true \
        @pparam=--fetch-policy=simple \
        @pparam=--fetch-path-pattern=/(.*-)(\d+)(.*)/$1{$2+2}$3/ \
        @pparam=--fetch-count=3

Compared to the example in the docs, I added --front=true.

@ywkaras ywkaras linked an issue May 3, 2022 that may be closed by this pull request
Prefetch Plugin Overflows when adding greater than 32-bit numbers #8569
Closed
@ywkaras
Copy link
Contributor

ywkaras commented May 3, 2022

Merging this for Mo Chen since he's not yet a committer.

@ywkaras ywkaras merged commit bd15558 into apache:master May 9, 2022
zwoop pushed a commit that referenced this pull request May 16, 2022
@moonchen @zwoop
Fix overflow conditions in prefetch plugin (#8660)
041e55f 
* Prefetch plugin: improve over/underflow handling

1. Specify integer size limitations.
2. Specify underflow/overflow behavior.
3. Refactor and add unit tests for evaluate().

* Use superscript for exponent

* Add end-of-line to evaluate.h

(cherry picked from commit bd15558)
@zwoop
Copy link
Contributor

zwoop commented May 16, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 May 16, 2022
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
f09493b 
* asf/9.2.x:
  Updated ChangeLog
  Fix parent_select optional scheme (apache#8831)
  Add `#pragma once` for PendingAction.h (apache#8846)
  Promote class PendingAction from HttpSM.h for use in other classes. (apache#8423)
  Fixes leak in SNIAction name globbing (apache#8827)
  Handle opentelemetry-cpp v1.3.0 upgrade for otel_tracer plugin (apache#8834)
  Fix overflow conditions in prefetch plugin (apache#8660)
  Remove incorrect comment from base64 functions (apache#8835)
  Add autest to cover updates to cache with alternates (apache#8779)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywkaras ywkaras ywkaras approved these changes

@bryancall bryancall Awaiting requested review from bryancall

Assignees

@moonchen moonchen

Labels
Plugins
Projects
None yet
Milestone
9.2.0
Development

Successfully merging this pull request may close these issues.

Prefetch Plugin Overflows when adding greater than 32-bit numbers
4 participants
@moonchen @masaori335 @ywkaras @zwoop