NGXZS / pe

0 stars 0 forks source link

Integer overflow #7

Open NGXZS opened 3 months ago

NGXZS commented 3 months ago

Description

Reproduction

add retail item i1 as shown below add -re -n re1 -d d1 -c 1 -q 2 then restock restock -n re1 -q 2147483647 restock -n re1 -q 2

Error Message

Actual

image.png

image.png

Expected

negative quantity does not make sense

soc-se-bot commented 2 months ago

Team's Response

A duplicate of #854.

image.png

This is clearly a case of integer overflow, as you have accurately pointed out in the issue title. This is caused by extreme user behavior, as we do not expect our target users (small business owners) to have this much amount of stock.

To put things into perspective, 2,147,483,647 is 2.147 billion. It is incredibly unlikely for any small business to have 2.147 billion of a certain item in stock.

Furthermore, it does not cause our app to crash even if users behave this way, and hence, it is unjustified to claim that this is a High severity bug.

The 'Original' Bug

[The team marked this bug as a duplicate of the following bug]

Integer overflow for all whole number outputs

image.png


[original: nus-cs2113-AY2324S2/pe-interim#777] [original labels: type.FunctionalityBug severity.High]

Their Response to the 'Original' Bug

[This is the team's response to the above 'original' bug]

Clearly an example of extreme user behavior. Noted on the attempts to try some sort of input injection attacks as well 🙄.

image.png

Anyway, there is obviously no integer overflow in this case. The extremely large threshold value, which you tried to input, was correctly caught by the application and treated as invalid.

How can you therefore say that there is an overflow when your input has been invalidated and not accepted by the application?

High bug severity is completely unjustified and unwarranted as well, since there is no concrete proof of "integer overflow" in your attached screenshot. I'll reduce its severity to Low.

Items for the Tester to Verify

:question: Issue duplicate status

Team chose to mark this issue as a duplicate of another issue (as explained in the Team's response above)

Reason for disagreement: There are a few differences with the duplicate titled #854. Please see below

Duplicate Screenshot:

image.png

My bug report Screenshot:

image.png

(Note: please see screenshots above/ original bug reports for verification)

Duplicate's core issue is not integer overflow

The duplicate issue is reproduced by inputting a very large number (eg 9999...99) as an input to the threshold parameter (as indicated by the flag -t), which is rejected by the program.

The issue I report is reproduced by inputting 2 numbers (eg 2147483647 and 2) that are accepted as inputs (ie not rejected by program) to the quantity parameter (as indicated by the flag -q) and which consequently causes integer overflow (eg quantity: -2147483647) by the program's internal logic calculation in the restock feature

Different commands and parameters

As seen in the screenshots above, the duplicate tests the add feature and threshold parameter (-t). The issue I reported is testing the restock feature and quantity parameter (-q). Since both reports are about 2 different features and 2 different parameters, they are different.

Different error messages

As mentioned above, duplicate's inputs are rejected by the program. The inputs in my bug reports are accepted by the program and have no error messages. (see screenshots above/ original bug reports)

The team's response to both bug reports

The team mentioned the duplicate "obviously (has) no integer overflow in this case", whereas the issue I reported "is clearly a case of integer overflow". (see first 2 sentences of both bug reports)


## :question: Issue response Team chose [`response.Rejected`] - [x] I disagree **Reason for disagreement:** Team rejected due to 3 main reasons: 1. extreme user input (in the case of deliberate sabotage eg 30 digit long telephone number, which is also not possible) 2. target user group are small businesses which may not have 2.147 billion of a certain item in stock. 3. Does not cause program to crash, so should not be `severity.High` (this I agree) ## With regards to reason #1 (fyi `2,147,483,647` is largest value an Integer can hold in Java (ie INTEGER.MAX_VALUE). A `+1` will cause the result to overflow and result in a negative number) My inputs were `2,147,483,647` and `2`, which I admit were not the best inputs under timed conditions. Consider if the inputs were of a more reasonable number (eg `1,500,000,000` and `1,500,000,000`). This will also cause the same error as both inputs are not more than `2,147,483,647` due to the INTEGER.MAX_VALUE reason above. Additionally, `1,500,000,000` may be a feasible quantity number in some industries, so it may not be considered "extreme user input` for these industries. (see explanation below) ## With regards to reason #2 Consider a small business that **assembles electric products/ components from the gate level.** A single electric product may involve **as many as 300000 components**, each with as many as 100 logic gates. (see https://www.daenotes.com/electronics/devices-circuits/integrated-circuits-ic) There may be other examples that require a large number of quantity, but this is just 1 of them. In this electric product business example, 1 product has at least 300000 components. Businesses normally **mass produce**. Hence, total quantity of these products can easily surpass the INTEGER.MAX_VALUE in these cases. ## With regards to reason #3 Nonetheless, I can see how this may be business/ industry specific (ie in industries that uses a large number of a specific item). Considering the rarity, I agree that it should not be `severity.High`, but a `severity.Low`, as what the team suggested.
## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.High`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]