On 06/02/2025 00:11, Anton Moryakov wrote:
report
Possible integer overflow: left operand is tainted.
An integer overflow may occur due to arithmetic operation (addition)
between variable 'readsize' and value '1', when 'readsize'
is tainted { [-2147483648, -2], [0, 2147483647] }
Corrections explained:
- Combined error and overflow checks into a single condition: `if (readsize == -1
|| readsize >= INT_MAX)`.
- Set `errno` to `ENAMETOOLONG` in case of overflow.
- Improved code readability and safety.
The explanations do not match the patch.
What situation are you trying to guard against here? Are you somehow
managing to construct a symbolic link where the link's target is larger
than INT_MAX characters? If it is possible for that to happen, then the
code already has bigger issues: a loop where the buffer size is
increased by 80 bytes every attempt is going to be a very slow way of
handling that.
Regardless, the patch is not right: there are three scenarios for
xmalloc_readlink:
1: The path could not be determined to be a symbolic link. This is
indicated by a NULL return value.
2: The path could be determined to be a symbolic link, and the target
was read. This is indicated by a non-NULL return value.
3: The path could be determined to be a symbolic link, and the target
could not be read because it was larger than what busybox can handle.
This is indicated by busybox exiting (from inside xrealloc).
Your change makes it so that a hypothetical symbolic link that is larger
than INT_MAX characters gets treated as case 1, when it should be
treated as case 2 or 3.
Cheers,
Harald van Dijk
_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox
