Skip to content

ZOOKEEPER-4281: Allow packet of max packet length to be deserialized #1683

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
Closed
wants to merge 2 commits into from
Closed

ZOOKEEPER-4281: Allow packet of max packet length to be deserialized #1683

wants to merge 2 commits into from

Conversation

@huizhilu
Copy link
Contributor

@huizhilu huizhilu commented Apr 13, 2021 •
edited
Loading

Problem

Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen. It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.

if (len < 0 || len >= packetLen) {
    throw new IOException("Packet len " + len + " is out of range!");
}

Solution

Change: len >= packetLen -> len > packetLen

if (len < 0 || len > packetLen) {

@huizhilu huizhilu changed the title Allow packet of max packet length to be deserialized [ZOOKEEPER-4281] Allow packet of max packet length to be deserialized Apr 13, 2021
@huizhilu huizhilu changed the title [ZOOKEEPER-4281] Allow packet of max packet length to be deserialized ZOOKEEPER-4281: Allow packet of max packet length to be deserialized Apr 13, 2021
@huizhilu
Copy link
Contributor Author

huizhilu commented Apr 13, 2021

@eolivelli @maoling @ztzg @hanm Could you help take a quick look and decide whether the PR is valid/helpful? Thank you!

@eolivelli
Copy link
Contributor

eolivelli commented Apr 14, 2021

It makes sense to me

arshadmohammad
arshadmohammad approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@arshadmohammad arshadmohammad left a comment

Choose a reason for hiding this comment

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

LGTM +1

@huizhilu
Copy link
Contributor Author

huizhilu commented Apr 14, 2021

Thanks for the review, @eolivelli, @arshadmohammad.

Failed CI checks don't seem related to this change.

retest maven build

eolivelli
eolivelli requested changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Sorry for late reply.

Can you please add one unit test around the code you changed? In order to not introduce regressions in the future

@huizhilu
Copy link
Contributor Author

huizhilu commented Apr 16, 2021

Sorry for late reply.

Can you please add one unit test around the code you changed? In order to not introduce regressions in the future

@eolivelli Thanks for the suggestion. A unit test is added :) Could you take another look?

@huizhilu huizhilu requested a review from eolivelli April 16, 2021 22:25
eolivelli
eolivelli approved these changes Apr 17, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Perfect

@huizhilu
Copy link
Contributor Author

huizhilu commented Apr 20, 2021

@eolivelli @arshadmohammad @maoling Is it fine to merge this PR? Let me know if there is anything else we should do before merging it. Thank you!

@eolivelli
Copy link
Contributor

eolivelli commented Apr 20, 2021

I have restarted CI jobs, as soon as CI is green I will merge.
sorry, it is taking so long

@huizhilu
Copy link
Contributor Author

huizhilu commented Apr 21, 2021

I have restarted CI jobs, as soon as CI is green I will merge.
sorry, it is taking so long

@eolivelli thanks for triggering the checks. It seems a check (tests) timed out. From the previous merged PRs, the tests are not stable. It might need several retries to get all checks green :(

@eolivelli
Copy link
Contributor

eolivelli commented Apr 21, 2021

I have restarted the CI job, we still have a few problems with GitHub actions and JDK11, sorry

@eolivelli eolivelli closed this Apr 21, 2021
@eolivelli eolivelli reopened this Apr 21, 2021
@eolivelli
Copy link
Contributor

eolivelli commented Apr 21, 2021

it looks like the link to GH actions diappeared

btw this fix is save and main QA checks passed.
merging it now

@eolivelli eolivelli closed this in 1590a42 Apr 21, 2021
eolivelli pushed a commit that referenced this pull request Apr 21, 2021
@huizhilu
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
1a4cf4d 
### Problem
Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
```
if (len < 0 || len >= packetLen) {
    throw new IOException("Packet len " + len + " is out of range!");
}
```

### Solution
Change: `len >= packetLen`   ->  `len > packetLen`
```
if (len < 0 || len > packetLen) {
```

Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

Closes #1683 from pkuwm/maxPacketLength

(cherry picked from commit 1590a42)
Signed-off-by: Enrico Olivelli <eolivelli@datastax.com>
@eolivelli
Copy link
Contributor

eolivelli commented Apr 21, 2021

committed to master branch (3.8.x) and to branch-3.7 (3.7.1)

huizhilu added a commit to huizhilu/zookeeper that referenced this pull request Apr 21, 2021
@huizhilu
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
c4323b0 
Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
```
if (len < 0 || len >= packetLen) {
    throw new IOException("Packet len " + len + " is out of range!");
}
```

Change: `len >= packetLen`   ->  `len > packetLen`
```
if (len < 0 || len > packetLen) {
```

Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

Closes apache#1683 from pkuwm/maxPacketLength
huizhilu added a commit to huizhilu/zookeeper that referenced this pull request Apr 21, 2021
@huizhilu
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized
8538ff4 
Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
```
if (len < 0 || len >= packetLen) {
    throw new IOException("Packet len " + len + " is out of range!");
}
```

Change: `len >= packetLen`   ->  `len > packetLen`
```
if (len < 0 || len > packetLen) {
```

Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

Closes apache#1683 from pkuwm/maxPacketLength
huizhilu added a commit to linkedin/zookeeper that referenced this pull request Apr 21, 2021
@huizhilu
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized (#…
507da1a 
…48)

    ZOOKEEPER-4281: Allow packet of max packet length to be deserialized

    ### Problem
    Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

    There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
    ```
    if (len < 0 || len >= packetLen) {
        throw new IOException("Packet len " + len + " is out of range!");
    }
    ```

    ### Solution
    Change: `len >= packetLen`   ->  `len > packetLen`
    ```
    if (len < 0 || len > packetLen) {
    ```

    Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

    Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

    Closes apache#1683 from pkuwm/maxPacketLength
narendly pushed a commit to linkedin/zookeeper that referenced this pull request Nov 23, 2021
@huizhilu @narendly
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized (#…
d62d8b4 
…48)

    ZOOKEEPER-4281: Allow packet of max packet length to be deserialized

    ### Problem
    Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

    There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
    ```
    if (len < 0 || len >= packetLen) {
        throw new IOException("Packet len " + len + " is out of range!");
    }
    ```

    ### Solution
    Change: `len >= packetLen`   ->  `len > packetLen`
    ```
    if (len < 0 || len > packetLen) {
    ```

    Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

    Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

    Closes apache#1683 from pkuwm/maxPacketLength
adityaagg09 pushed a commit to linkedin/zookeeper that referenced this pull request Nov 19, 2025
@huizhilu
ZOOKEEPER-4281: Allow packet of max packet length to be deserialized (#…
4d861b8 
…48)

    ZOOKEEPER-4281: Allow packet of max packet length to be deserialized

    ### Problem
    Resolves https://issues.apache.org/jira/browse/ZOOKEEPER-4281

    There are sanity checks for packet size when deserializing a packet. One place has the inclusion: len >= packetLen.  It rejects to deserialize the bytes that are exactly sized jute.maxbuffer. It's confusing. This check should use ">" so the maxbuffer length packet can still be deserialized, like the other checks.
    ```
    if (len < 0 || len >= packetLen) {
        throw new IOException("Packet len " + len + " is out of range!");
    }
    ```

    ### Solution
    Change: `len >= packetLen`   ->  `len > packetLen`
    ```
    if (len < 0 || len > packetLen) {
    ```

    Author: Huizhi Lu <5187721+pkuwm@users.noreply.github.com>

    Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

    Closes apache#1683 from pkuwm/maxPacketLength
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanm hanm hanm approved these changes

@arshadmohammad arshadmohammad arshadmohammad approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@huizhilu @eolivelli @hanm @arshadmohammad