Bug 1003986

Summary: FTBFS: libcomps-0.1.3-2.fc20 tests fail on power & s390
Product: [Fedora] Fedora Reporter: David Aquilina <dwa>
Component: libcompsAssignee: Jindrich Luza <jluza>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: urgent    
Version: 20CC: dan, flanagan, gustavold, jluza, karsten
Target Milestone: ---   
Target Release: ---   
Hardware: powerpc   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-06 09:45:08 EDT Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 467765, 1003989    
Description Flags
libcomps-0.1.3-2.fc20 build log none

Description David Aquilina 2013-09-03 12:17:21 EDT
Created attachment 793269 [details]
libcomps-0.1.3-2.fc20 build log

Description of problem:

Some libcomps tests fail when trying to build on power, e.g.: 

in comps_parse test: 

WARNING: Unknown elemenet 'O$`�' at line:-7209592
'OX' found between elements at line:-7209560 column :-7209564
No declaration for element packageFOO
*** Error in `./test_parse': free(): invalid next size (fast): 0x104f5698 ***

Attaching the full build log (and also it's available from http://ppc.koji.fedoraproject.org/kojifiles/work/tasks/114/1390114/build.log
Comment 1 Gustavo Luiz Duarte 2013-09-05 11:29:33 EDT
"test_default" fails because the variable "def" is not initialized on libcomps/src/python/pycomps_gids.c:421.

From http://docs.python.org/dev/c-api/arg.html:

|: Indicates that the remaining arguments in the Python argument list are optional. The C variables corresponding to optional arguments should be initialized to their default value — when an optional argument is not specified, PyArg_ParseTuple() does not touch the contents of the corresponding C variable(s).

Non-static local variables are not automatically initialized. Please initialize def with the default value (which I believe to be zero, inferring from the test case).

"test_fedora" is failing due to misuse of sprintf() on ./libcomps/src/comps_logger.c:226.

The type of the arguments passed to sprintf() must match the corresponding specifier character in the format string, otherwise the result is non-portable and has undefined behavior.
On comps_logger.c:226, the arguments passed to sprintf() have the type union, even though the format string expects a pointer to char and a few integers.

We need to rework the logic on comps_log_entry_str() to make it pass the arguments to sprintf() with the correct type.

A quick workaround is to change the type of pitem.i from int to intptr_t. Even though this workaround makes the test pass on ppc64, it is still non-portable and I have not tested on x86 or arm.

Another thing to note is that this code is always passing 4 arguments to sprintf(), even though there might be only 3 specifier characters. AFAIK, passing the wrong number of arguments to sprintf() also has undefined behavior and should be avoided, even though it doesn't cause any issue on ppc64 with current glibc.

Even though we might go with the workaround I mentioned to unblock the composes on ppc64, I strongly suggest we properly fix this issue ASAP by reworking comps_log_entry_str() to make it pass sprintf() arguments appropriately.
Comment 2 Gustavo Luiz Duarte 2013-09-05 11:34:27 EDT
Forgot to mention, there are still a couple of tests failing on ppc64: test_xml and test_union3. I will look at it later.
Comment 3 Gustavo Luiz Duarte 2013-09-05 16:13:18 EDT
The test_xml and test_union3 test failures seem to be caused by another misuse of PyArg_ParseTuple() on libcomps/src/python/pycomps_groups.c:1097.
The format string passed to PyArg_ParseTuple() is "|si", indicating two optional arguments, a pointer to char and an integer. But instead of an integer, a long argument has been passed.
Please change either the type of the variable or the format string so that they match.
Comment 4 Gustavo Luiz Duarte 2013-09-05 16:31:41 EDT
With fixes 1, 2 (workaround) and 3 in place I was able to build libcomps successfully on ppc64. I can try to come up with a proper fix for item #2, though anyone with more familiarity with libcomps code is very welcome to jump in here and write it.
Comment 5 Jindrich Luza 2013-09-06 09:45:08 EDT
I fixed problems above by Gustavo Luiz Duarte guidelines (except 2. - no workaround) and build on ppc passed. Here is task info: 
And I just submited package update, so fix will be in libcomps-0.1.3-3.fc20.

Issue no.2 is fixed by rewriting comps_log_entry_str function.I passing all strings to sprintf now, and I also changed all format string specifiers to %s, so it shouldn't be problem in passing differ type as sprintf arguments anymore.
Wrong argument number passing to sprintf is solved by counting argument usage and then calling sprintf with appropriate arguments (not much elegant but works for now - whole libcomps c-python binding architecture is designated to rewrite according to https://github.com/midnightercz/libcomps/issues/11 anyway).

And also big thanks to Gustavo Luiz Duarte. Because within his investigations, I probably had to wait for ppc arch reservation (I haven't any ppc machine avaible) and whole proccess took much more time.
Comment 6 Gustavo Luiz Duarte 2013-09-06 10:26:06 EDT
Great! Thanks Jindrich.