Skip to content

cats: fix for integer overflow issue when using offset in llist#1547

Merged
BareosBot merged 7 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/s5534-false-integer-conversion
Oct 6, 2023
Merged

cats: fix for integer overflow issue when using offset in llist#1547
BareosBot merged 7 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/s5534-false-integer-conversion

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Sep 12, 2023

Description

This PR is a bug fix for the mantis bug 1027

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
    Check backport line
  • Required backport PRs have been created
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
Tests
  • Decision taken that a test is required (if not, then remove this paragraph)
  • The choice of the type of test (unit test or systemtest) is reasonable
  • Testname matches exactly what is being tested
  • On a fail, output of the test leads quickly to the origin of the fault

@alaaeddineelamri alaaeddineelamri changed the title cats: fix for integer overflow issue when creating OFFSET for a query cats: fix for integer overflow issue when using offset in llist Sep 12, 2023
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

I think we should change it to 64bits; what do you think ?

@sebsura sebsura self-assigned this Sep 14, 2023
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch from 3ab6fa6 to d8643ea Compare September 14, 2023 08:14
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

You need to change the commit message too!

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch from d8643ea to a6f0783 Compare September 14, 2023 08:33
Copy link
Contributor

@sebsura sebsura 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. During testing i realised that std::stoull throws an exception if the value is out of range which could crash the director.
This can be prevented with a simple try {} catch (...) {}.

Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

We should also maybe have a test (in bareos-basic for example) that just executes a nonsense query (i.e. limit and offset way to high) and check that it handles it correctly (both for llist and list).

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch from a6f0783 to 302603d Compare September 19, 2023 08:45
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch 4 times, most recently from 0bd62f7 to 4c0a013 Compare September 21, 2023 10:45
@sebsura sebsura added the onhold label Sep 22, 2023
@sebsura sebsura removed the onhold label Oct 5, 2023
@sebsura sebsura force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch from 97364d3 to 9f285c2 Compare October 6, 2023 04:44
@sebsura sebsura added the bugfix label Oct 6, 2023
@BareosBot BareosBot force-pushed the dev/alaaeddineelamri/master/s5534-false-integer-conversion branch from 736ef5a to 6068506 Compare October 6, 2023 08:44
@BareosBot BareosBot merged commit 8ae192e into bareos:master Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants