Bug 1696458 - PString Mid() fails due to gcc signed int overflow optimization breaking Ekiga
Summary: PString Mid() fails due to gcc signed int overflow optimization breaking Ekiga
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: ptlib
Version: 29
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Peter Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-04-04 21:29 UTC by Krzysztof Halasa
Modified: 2019-05-30 12:51 UTC (History)
4 users (show)

Fixed In Version: ptlib-2.10.11-3.fc30
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-05-30 12:51:34 UTC
Type: Bug


Attachments (Terms of Use)

Description Krzysztof Halasa 2019-04-04 21:29:43 UTC
Description of problem:
PString Mid() tries to detect signed integer overflow, but the test is 'undefined behaviour' and gcc happily optimizes it out:

PString PString::Mid(PINDEX start, PINDEX len) const
{
  if (len <= 0 || start < 0)
    return Empty();

  if (start+len < start) // Beware of wraparound
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Recent gcc optimizes the above to if (len < 0)

    return operator()(start, P_MAX_INDEX);
  else
    return operator()(start, start+len-1);
}

This breaks (at least) Ekiga's SIP.

Version-Release number of selected component (if applicable):
ptlib-2.10.11-1.fc29.x86_64

How reproducible:
always

Steps to Reproduce:
1. Setup 2 computers with Ekiga on each
2. run ekiga -d 8 on each computer
3. try to call the 2nd computer using sip:IP_address

Actual results:
The 2nd Ekiga ignores the call, debug output contains information about invalid version of SIP packet:
SIP     Invalid version (0) received on udp$x.x.x.x:5060<if=udp$*:5060>

Expected results:
The 2nd Ekiga should ring.

Additional info:
A quick and dirty workaround would be adding -fwrapv to gcc options.

Comment 1 Robert Scheck 2019-05-14 23:19:42 UTC
Could you try to replace

> if (start+len < start) // Beware of wraparound

by

> if (len == P_MAX_INDEX || start+len < start) // If open ended or check for wraparound

and test whether it works?

Comment 2 Jonathan Wakely 2019-05-16 14:00:07 UTC
Or change it to 'if (start > INT_MAX - len)'

Comment 3 Jonathan Wakely 2019-05-16 14:01:22 UTC
Oh but it's not an int. So then:

  if (start > std::numeric_limits<PINDEX>::max() - len)

Comment 4 Krzysztof Halasa 2019-05-20 18:49:58 UTC
Both first
 if (len == P_MAX_INDEX || start+len < start) // If open ended or check for wraparound

and the second
 #include <limits>
 if (start > std::numeric_limits<PINDEX>::max() - len)

work.

OTOH I'm not sure the first test is entirely valid in case of a generic library like this. len can be a bit less than P_MAX_INDEX (it's just an arbitrary limit) and still cause the "start + len - 1" to fail.
The second test guarantees that start <= max - len and thus start + len - 1 < max.

Thanks for looking into this.

Comment 5 Robert Scheck 2019-05-20 20:17:40 UTC
My suggestion in comment #2 is simply from what latest upstream uses. If we need to propose something better to upstream, I would need your help in order to do so.

Comment 6 Robert Scheck 2019-05-20 20:18:16 UTC
Sorry, I meant comment #1 rather comment #2.

Comment 7 Fedora Update System 2019-05-21 08:11:25 UTC
ekiga-4.0.1-44.fc30 ptlib-2.10.11-3.fc30 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-c5e3dc44ba

Comment 8 Krzysztof Halasa 2019-05-21 16:09:14 UTC
Right. I just wasn't aware that the ptlib version used by Ekiga is 7+ years old.

Comment 9 Fedora Update System 2019-05-22 01:44:08 UTC
ekiga-4.0.1-44.fc30, ptlib-2.10.11-3.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-c5e3dc44ba

Comment 10 Fedora Update System 2019-05-30 12:51:34 UTC
ekiga-4.0.1-44.fc30, ptlib-2.10.11-3.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.