Bug 1023322 - qpid c++ client Address string compatibility regression in parsing (long) int value postfixed by 'L'
qpid c++ client Address string compatibility regression in parsing (long) int...
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise MRG
Classification: Red Hat
Component: qpid-cpp (Show other bugs)
Development
Unspecified Unspecified
medium Severity high
: 3.0
: ---
Assigned To: Gordon Sim
Frantisek Reznicek
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-25 04:07 EDT by Frantisek Reznicek
Modified: 2015-01-21 07:52 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
It was discovered that numeric queue configuration options specified as strings were not checked when parsed, to ensure there were no trailing non-digit characters after the number. Values such as 40000L were previously accepted as was 40000xyz, with the trailing characters simply being ignored. This may have given the incorrect impression in some cases that values such as 40000L were recognized as being a long integer. The parsing logic when converting from string to numeric option now ensures there are no illegal trailing characters. Values such as 40000L are now rejected as invalid.
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-01-21 07:52:28 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
change to allow (and ignore) c style integer literal suffixes (5.94 KB, patch)
2013-10-25 11:49 EDT, Gordon Sim
no flags Details | Diff

  None (edit)
Description Frantisek Reznicek 2013-10-25 04:07:24 EDT
Description of problem:

There is observed qpid c++ client Address string compatibility regression in parsing long int postfixed by L. Let's have following two address string examples:

a] "queue_tc001/bindkey_tc001; {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500L,  qpid.flow_resume_size: 400L,  qpid.flow_stop_count: 500L,  qpid.flow_resume_count: 400L,  }}}}"

b] "queue_tc001/bindkey_tc001; {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500,  qpid.flow_resume_size: 400,  qpid.flow_stop_count: 500,  qpid.flow_resume_count: 400,  }}}}"

Both variants are accepted by stable released MRG/M, but Vienna candidate accepts just the latter one. The first one is considered as illegal - following invalid conversion is thrown:

# ${QPID_SEND}  --messages 1 --content-size 0 --capacity 1 --address "queue_tc001/bindkey_tc001;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500L,  qpid.flow_resume_size: 400L,  qpid.flow_stop_count: 500L,  qpid.flow_resume_count: 400L,  }}}}"
2013-10-25 09:54:11 [Client] warning Exception received from broker: invalid-argument: invalid-argument: invalid conversion: Cannot convert 400L (/builddir/build/BUILD/qpid-0.22/cpp/src/qpid/types/Variant.cpp:132) [caused by 2 \x08:\x01]
qpid-send: invalid-argument: invalid-argument: invalid conversion: Cannot convert 400L (/builddir/build/BUILD/qpid-0.22/cpp/src/qpid/types/Variant.cpp:132)
2013-10-25 09:54:11 [Client] warning Ignoring frame while closing connection: Frame[BEbe; channel=1; {SessionDetachBody: name=bb05b403-2d99-4d7c-abc7-b26fcbc511d2; }]
[root@dhcp-27-116 ~]# echo $?
1

Current understanding is that on AMQP 0-10 there will be complete backword compatibility of address string.
If this is not the case (in this particular scenario) then it is expected that restriction will be documented explicitly.


Version-Release number of selected component (if applicable):
qpid-cpp-*0.22-13.el5
qpid-cpp-*0.22-21.el6

How reproducible:
100%

Steps to Reproduce:
1. execute below attached (Additional info) BASH snippet

Actual results:
AMQP0-10 address string is not 100% compatible.

Expected results:
AMQP0-10 address string should be 100% compatible.

Additional info:

Testing snippet:
service qpidd restart

QPID_SEND=$(which qpid-send)
[ -z "${QPID_SEND}" ] && QPID_SEND=/opt/rh-qpid/clients/qpid-send

${QPID_SEND}  --messages 1 --content-size 0 --capacity 1 --address "queue_tc001/bindkey_tc001;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500L,  qpid.flow_resume_size: 400L,  qpid.flow_stop_count: 500L,  qpid.flow_resume_count: 400L,  }}}}"
echo $?

${QPID_SEND} --messages 1 --content-size 0 --capacity 1 --address "queue_tc002/bindkey_tc002;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500,  qpid.flow_resume_size: 400,  qpid.flow_stop_count: 500,  qpid.flow_resume_count: 400,  }}}}"
echo $?


Execution on stable release:
[root@dhcp-27-148 ~]# QPID_SEND=$(which qpid-send)
/usr/bin/which: no qpid-send in (/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin:/root/bin)
[root@dhcp-27-148 ~]# [ -z "${QPID_SEND}" ] && QPID_SEND=/opt/rh-qpid/clients/qpid-send
[root@dhcp-27-148 ~]# 
[root@dhcp-27-148 ~]# ${QPID_SEND}  --messages 1 --content-size 0 --capacity 1 --address "queue_tc001/bindkey_tc001;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500L,  qpid.flow_resume_size: 400L,  qpid.flow_stop_count: 500L,  qpid.flow_resume_count: 400L,  }}}}"
[root@dhcp-27-148 ~]# echo $?
0
[root@dhcp-27-148 ~]# 
[root@dhcp-27-148 ~]# ${QPID_SEND} --messages 1 --content-size 0 --capacity 1 --address "queue_tc002/bindkey_tc002;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500,  qpid.flow_resume_size: 400,  qpid.flow_stop_count: 500,  qpid.flow_resume_count: 400,  }}}}"
[root@dhcp-27-148 ~]# echo $?
0

Execution on Vienna candidate release:
[root@dhcp-27-117 ~]# QPID_SEND=$(which qpid-send)
[root@dhcp-27-117 ~]# [ -z "${QPID_SEND}" ] && QPID_SEND=/opt/rh-qpid/clients/qpid-send
[root@dhcp-27-117 ~]# 
[root@dhcp-27-117 ~]# ${QPID_SEND}  --messages 1 --content-size 0 --capacity 1 --address "queue_tc001/bindkey_tc001;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500L,  qpid.flow_resume_size: 400L,  qpid.flow_stop_count: 500L,  qpid.flow_resume_count: 400L,  }}}}"
2013-10-25 09:54:15 [Client] warning Exception received from broker: invalid-argument: invalid-argument: invalid conversion: Cannot convert 400L (/builddir/build/BUILD/qpid-0.22/cpp/src/qpid/types/Variant.cpp:132) [caused by 2 \x08:\x01]
qpid-send: invalid-argument: invalid-argument: invalid conversion: Cannot convert 400L (/builddir/build/BUILD/qpid-0.22/cpp/src/qpid/types/Variant.cpp:132)
2013-10-25 09:54:15 [Client] warning Ignoring frame while closing connection: Frame[BEbe; channel=1; {SessionDetachBody: name=891b29b0-ea3a-42c9-b506-16379a0a6ca7; }]
[root@dhcp-27-117 ~]# echo $?
1
[root@dhcp-27-117 ~]# 
[root@dhcp-27-117 ~]# ${QPID_SEND} --messages 1 --content-size 0 --capacity 1 --address "queue_tc002/bindkey_tc002;         {create: always, node:{ x-declare :{arguments :{ qpid.flow_stop_size: 500,  qpid.flow_resume_size: 400,  qpid.flow_stop_count: 500,  qpid.flow_resume_count: 400,  }}}}"
[root@dhcp-27-117 ~]# echo $?
0
Comment 1 Gordon Sim 2013-10-25 11:47:26 EDT
This is not in fact a regression in my view. In the older code, 400L is indeed accepted but so is 400Bananas or 400+8 or 400k and these are all treated as the same value.

I.e. the conversion prior to qpid 0.20 did not check for any trailing characters. So in reality the older code was broken and it is just a coincidence that the lack of precision in conversion makes it look like it accepts the c style integer suffixes.

I have prepared a patch that would explicitly allow for such suffixes. However I do wonder whether there is actually any value in this and what the expectation is in terms of handling them. (The patch simply ignores any valid c style suffix when converting). As far as I can see there is no good reason for including such suffixes in a string.
Comment 2 Gordon Sim 2013-10-25 11:49:39 EDT
Created attachment 816223 [details]
change to allow (and ignore) c style integer literal suffixes

though as mentioned in the previous comment, I'm not convinced there is any value in making this change.
Comment 3 Justin Ross 2013-10-25 12:24:49 EDT
Okay, let's *not* make this change.  The existing behavior is superior.  I'll leave this open so we can document it.

(In reply to Gordon Sim from comment #2)
> Created attachment 816223 [details]
> change to allow (and ignore) c style integer literal suffixes
> 
> though as mentioned in the previous comment, I'm not convinced there is any
> value in making this change.
Comment 4 Justin Ross 2013-11-14 07:04:38 EST
Flagged for a release note.  See Gordon's comment 1 for the explanation.
Comment 6 Frantisek Reznicek 2013-11-19 11:44:26 EST
I agree with no fix + release notes + migration document.
Comment 7 Frantisek Reznicek 2014-08-20 06:51:02 EDT
This defect is MODIFIED to allow QA to verify release note and migration document are updated accordingly.
Comment 10 Frantisek Reznicek 2014-08-26 09:40:33 EDT
Issue release-noted in Vienna docs (2.1.  Known Issues in MRG-M 3.0). Not necessary to have it also in migration document.

-> VERIFIED
Comment 13 Frantisek Reznicek 2014-09-09 04:42:25 EDT
Hello Gordon,
may I possibly ask you for creating doctext (CCFR form) matching your comment 1, please? This defect is private and as Jared pointed out in comment 11 it would be optimal to have complete doctext.
Comment 17 Frantisek Reznicek 2014-09-22 03:26:44 EDT
Please find Group changed to None. Thank you Leonid. Clearing needinfo.
Comment 18 Frantisek Reznicek 2014-09-22 04:15:35 EDT
We reached acceptance criteria for this defect, defect is now public (includes doc text) and listed in release notes.

-> VERIFIED

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