4 ms·

I believe there is a bug in the bug fix. I've explained it here: https://blog.deckc.hair/2022-12-11-incorrect-bug-fix-for-24yr-old-ping-bug.html https://blog.d

I believe there is a bug in the bug fix. I've explained it here: https://blog.deckc.hair/2022-12-11-incorrect-bug-fix-for-24yr-old-ping-bug.html https://blog.deckc.hair/2022-12-11-incorrect-bug-fix-for-24y... In brief (cp[IPOPT_OLEN] - 1) is the delta, not (cp[IPOPT_OLEN]); according to the original code. (I don't pretend to know where the original -1 comes from). This means that the IF-statement check is incorrect.

charcircuit 4y ago
There is no bug. >delta is allowed to be zero, we make no progress (as in the original bug) It's not zero progress because the for loop increments cp every iteration. In the original bug delta = -1.
- nandalism 4y ago
  Thank you for the clarification. I did not look at the surrounding code. However, my point still stands that extracting the complex expression as delta would have helped and possibly avoided the original bug. int const delta = (cp[IPOPT_OLEN] - 1); Further proof is that the original author first commited a broken fix. Again this was caused by the confusion between delta and delta-1, which would have been clear had delta been explicitly named. This sort of code is just begging to be misunderstood. - if (cp[IPOPT_OLEN] > 0 && cp[IPOPT_OLEN] < hlen) { + if (cp[IPOPT_OLEN] > 0 && (cp[IPOPT_OLEN] - 1) <= hlen) {
  - charcircuit 4y ago
    The "bug" is from assuming you are parsing a valid packet. The option length field is the length of the option legnth and option data combined. The minimum value it can have is 1 which is when the length of the option data is 0. cp[IPOPT_OLEN] refers to the option length. Your delta variable is the length of the option data. The confusing part is that in most network protocols length fields do not count the size of the length number.
bdhcuidbebe 4y ago
It is interesting why you chose to write a blog article on a diff, without looking at it in context. What is your end game, lol.
- nandalism 4y ago
  :) I am glad to have enterained you. My end game is to get people to write int const delta = (cp[IPOPT_OLEN] - 1); instead of repeating expressions like this: (cp[IPOPT_OLEN] - 1) and adding variants elsewhere like this: cp[IPOPT_OLEN]. It's confusing and causes bugs. Naming things helps and costs nothing.

charcircuit 4y ago

There is no bug. >delta is allowed to be zero, we make no progress (as in the original bug) It's not zero progress because the for loop increments cp every iteration. In the original bug delta = -1.

nandalism 4y ago
Thank you for the clarification. I did not look at the surrounding code. However, my point still stands that extracting the complex expression as delta would have helped and possibly avoided the original bug. int const delta = (cp[IPOPT_OLEN] - 1); Further proof is that the original author first commited a broken fix. Again this was caused by the confusion between delta and delta-1, which would have been clear had delta been explicitly named. This sort of code is just begging to be misunderstood. - if (cp[IPOPT_OLEN] > 0 && cp[IPOPT_OLEN] < hlen) { + if (cp[IPOPT_OLEN] > 0 && (cp[IPOPT_OLEN] - 1) <= hlen) {
- charcircuit 4y ago
  The "bug" is from assuming you are parsing a valid packet. The option length field is the length of the option legnth and option data combined. The minimum value it can have is 1 which is when the length of the option data is 0. cp[IPOPT_OLEN] refers to the option length. Your delta variable is the length of the option data. The confusing part is that in most network protocols length fields do not count the size of the length number.

bdhcuidbebe 4y ago

It is interesting why you chose to write a blog article on a diff, without looking at it in context. What is your end game, lol.

nandalism 4y ago
:) I am glad to have enterained you. My end game is to get people to write int const delta = (cp[IPOPT_OLEN] - 1); instead of repeating expressions like this: (cp[IPOPT_OLEN] - 1) and adding variants elsewhere like this: cp[IPOPT_OLEN]. It's confusing and causes bugs. Naming things helps and costs nothing.