Bug 143450 - Data corruption when returning small structs containing a double from C++ via IPC
Summary: Data corruption when returning small structs containing a double from C++ via...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: sdk (show other bugs)
Version:
(earliest affected)
6.0.0.3 release
Hardware: All Linux (All)
: medium normal
Assignee: Stephan Bergmann
URL:
Whiteboard: target:7.3.0
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-20 07:24 UTC by Marc-Oliver Straub
Modified: 2021-08-09 15:40 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-Oliver Straub 2021-07-20 07:24:08 UTC
Description:
I noticed data corruption of the double value in a small struct when returning the struct as result via IPC.
struct XMyStruct {
  double field1;
  hyper field2;
}

interface XMyInterface {
  XMyStruct myMethod();
}

XMyStruct myMethod() {
  return XMyStruct(2.0, 3);
}

The integer member seems to be correctly transferred, but the double member seems to be corrupt. If the struct contains two doubles or 2 hyper values, the data is correct. If the order of the fields is changed (hyper first, then double) both fields are corrupt.
Increasing the size of the struct, eg. by adding another double member fixes the data corruption.

Steps to Reproduce:
Implement a C++ service, implementing XMyInterface as in description.
Implement a C++ client in a different process calling XMyInterface via IPC


Actual Results:
Observe that the values of the struct in C++ client are corrupted, depending on the order of the double/hyper members in the struct.

Expected Results:
Struct members should be correct, regardless of their order.


Reproducible: Always


User Profile Reset: No



Additional Info:
I suggest Stephan Bergmann to take a look. I assume the bug is in the lowlevel code near ./bridges/source/cpp_uno/gcc3_linux_x86-64/abi.cxx, perhaps in x86_64::fill_struct(...)
Comment 1 Julien Nabet 2021-07-21 19:37:37 UTC
The whole 6.X branch is EOL. 7.0.X will be EOL quite soon.
Do you reproduce this with last LO versoin 7.1.4 ?

Stephan: it seems, as the reporter indicated, you may indeed be interested in this one.
Comment 2 Marc-Oliver Straub 2021-07-22 05:33:31 UTC
Since we're still using 6.x, it's quite difficult for me to reproduce on 7.x - we haven't updated our product to that release yet.
I don't have a stripped down reproducer since I don't know how to setup such a scenario in the libreoffice source tree.
But I've taken a look at abi.cxx (which I assume to be causing the issue), and this don't seem to have changed in the 7.x source tree.
Comment 3 Julien Nabet 2021-07-25 11:27:29 UTC
Just taking a look at the bridges/source/cpp_uno/gcc3_linux_x86-64/abi.cxx, I noticed 
this enum:
71  enum x86_64_reg_class
72  {
73      X86_64_NO_CLASS,
74      X86_64_INTEGER_CLASS,
75      X86_64_INTEGERSI_CLASS,
76      X86_64_SSE_CLASS,
77      X86_64_SSESF_CLASS,
78      X86_64_SSEDF_CLASS,
79      X86_64_SSEUP_CLASS,
80      X86_64_X87_CLASS,
81      X86_64_X87UP_CLASS,
82      X86_64_MEMORY_CLASS
83  };
see https://opengrok.libreoffice.org/xref/core/bridges/source/cpp_uno/gcc3_linux_x86-64/abi.cxx?r=10d29c39#74

idem in bridges/source/cpp_uno/gcc3_macosx_x86-64/abi.cxx

When I Googled searched SSESF gcc, I found this:
enum x86_64_reg_class
  {
    X86_64_NO_CLASS,
    X86_64_INTEGER_CLASS,
    X86_64_INTEGERSI_CLASS,
    X86_64_SSE_CLASS,
    X86_64_SSESF_CLASS,
    X86_64_SSEDF_CLASS,
    X86_64_SSEUP_CLASS,
    X86_64_X87_CLASS,
    X86_64_X87UP_CLASS,
    X86_64_COMPLEX_X87_CLASS, <<<<<< no present in LO
    X86_64_MEMORY_CLASS
  };
see https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/i386.c

Is it expected we don't declare X86_64_COMPLEX_X87_CLASS ?

For the rest I must recognize I don't know anything about PS ABI, I've just leafed through this doc:
https://courses.cs.washington.edu/courses/cse351/12wi/supp-docs/abi.pdf
Comment 4 Commit Notification 2021-07-28 11:09:18 UTC
Stephan Bergmann committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ca344be7aabf88dddde38841e6af6292ece6829b

tdf#143450: Fix special fp+integer struct return case for gcc_*_x86-64

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Stephan Bergmann 2021-07-28 11:11:35 UTC
(In reply to Commit Notification from comment #4)
> It will be available in 7.3.0.

Even though this issue has been filed with "Version (earliest affected)" claiming "6.0.0.3 release", from looking at the code I strongly assume that this issue has been present since forever.  I do not plan to backport the fix to any older branches.
Comment 6 Marc-Oliver Straub 2021-07-30 12:40:27 UTC
Stephan, thank you for your changeset but the Hyper,Double case still fails, the value of the double is corrupt.

Enhancing the tests in testtools with a struct HyperDouble will show the issue.
Comment 7 Commit Notification 2021-08-02 16:22:35 UTC
Stephan Bergmann committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d5a100f59467d94945656b54563e5ecdde2ca724

tdf#143450: Special integre+fp struct return case needs a fix too, of course

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Stephan Bergmann 2021-08-02 16:27:48 UTC
facepalm :)
Comment 9 Marc-Oliver Straub 2021-08-09 15:40:48 UTC
Thank you, seems to work now!