some ismrmrd-tools scripts segfault on Ubuntu 12.04 and 14.04

[Ghislain Vaillant]

Hi everyone,

I have made new VMs with 12.04 and 14.04 and installed the ismrmrd-tools package from the ISMRMRD ppa [1]

On Ubuntu 12.04 amd64, ismrmrd_generate_cartesian_shepp_logan consistently segfaults, with the following stacktrace reported by apport:
ISMRMRD::NDArray<std:complex<float>>::getNumberOfElements() ()
ISMRMRD::fft2c(ISMRMRD::NDArray<std:complex<float> >&, boot) ()
main()

On both 12.04 and 14.04, ismrmrd_c_demo consistently segfaults, with the following stacktrace reported by apport:
realloc()
ismrmrd_make_consistent_image()
ismrmrd_read_image()
main()

Is this reproducible on your machines as well ?

[1] https://launchpad.net/~ghisvail/+archive/ubuntu/ismrmrd

[Souheil Inati]

I haven’t checked the c example in a while, so I’m not sure what is going on. I just checked on my two development machines, on the mac10.10 I don’t get this error, on ubuntu14.04 I do. So there is definitely something going on. We’ll take a look.

Haven’t looked into the generate_cartesisan_shepp_logan error. That program works fine for me on Mac10.10 and Ubuntu14.04.

-Souheil

> --
> You received this message because you are subscribed to the Google Groups "ISMRMRD" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to ismrmrd+u...@googlegroups.com.
> To post to this group, send email to ism...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/ismrmrd/6b763ebc-3763-4281-a524-addc0394f8b0%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Souheil Inati]

Okay, it turns out that on ubuntu (or is it gcc?), pointers are not initialized to void. Basically you have to use the ismrmrd_init_ndarray and ismrmrd_init_image functions before you can use those structs.
I have pushed a fix to the development branch. Could you try it out?

Thanks,
Souheil

[Michael Hansen]

No variables are ever initialized to anything. Pointers included.

------
Sent from a mobile device - please excuse brevity and/or typos.

> To view this discussion on the web visit https://groups.google.com/d/msgid/ismrmrd/483D23DD-6257-43F7-BCD1-B874FE9F53FD%40gmail.com.

[Souheil Inati]

What I mean is that when you declare a struct with a pointer as a member something random goes in there, that is foo.pointer has some value. If you force foo.pointer = NULL then realloc acts like malloc and everything is hunky dory. If you don’t, then all bets are off and you can try and realloc some chunk of memory that you do not own. On the mac (with clang), NOT explicitly setting it to NULL works without for this particular case. On Linux it segfaults. So the fix is to use the C API as it was intended to be used, i.e.
1) declare your image
ISMRMRD_image im;
2) call the init function on it to make sure the header is initialized and the pointers are set to NULL
ismrmrd_init_image(&im);
3) use it

It is possible that this example may have never worked on linux.

BTW, This is a C-API only issue. It’s C, you have a gun and you can blow off both your feet. In the C++ API, we call init in the constructor, so this can never happen.

-SI

> To view this discussion on the web visit https://groups.google.com/d/msgid/ismrmrd/03DCD916-81E8-4C85-A5EC-637502F151FA%40gmail.com.

[Michael Hansen]

What I am saying is that if you have a struct with members in it. They value (whether it be a pointer or otherwise) is undefined until initialized. The fact that it worked on the mac is pure luck. It should not work in general. The value of a pointer, member or not is not guaranteed to be anything in particular until it is initialized.

To view this discussion on the web visit https://groups.google.com/d/msgid/ismrmrd/76779515-E52F-47E6-9089-6ABEBB08209F%40gmail.com.

[Souheil Inati]

That's what I said. 

Sent from my iPhone

[Ghislain Vaillant]

@souheil, I have applied your patch to the 1.2.1 sources and refreshed the
packages in the PPA.

The binary packages should be built soon by the launchpad build bots. Thanks
for working on this.

Regarding the segfault of the ismrmrd_generate_cartesian_shepp_logan script,
which seems to be happening only on 12.04, do we actually care or should this
be investigated as well ?

[Souheil Inati]

We don’t have a 12.04 machine to test on and can’t reproduce the crash on the systems we have. Could give a full stack trace?
Maybe we could discuss on the google chat in a couple of hours.

> --
> You received this message because you are subscribed to the Google Groups "ISMRMRD" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to ismrmrd+u...@googlegroups.com.
> To post to this group, send email to ism...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/ismrmrd/4933b46a-9966-4010-9f59-9ebbd1716297%40googlegroups.com.

[Michael Hansen]

We still have 12.04 on Vatican and there is always AWS.

------
Sent from a mobile device - please excuse brevity and/or typos.

[Souheil Inati]

Oh. I thought Vatican had been upgraded. I'll give it a try in a bit.

Sent from my iPhone

[Joseph Naegele]

It doesn't segfault when compiled in Debug mode (-g -O0) and it does when compiled in Release (-DNDEBUG -O3). Looking into it more now.

[Joseph Naegele]

OK it appears to be an error with the C++ compiler on Ubuntu 12 (once again, only with -O3).
Adding a dummy statement to prevent loop optimization prevents the GPF:

template <typename T> size_t NDArray<T>::getNumberOfElements() {
     size_t num = 1;
     for (int n = 0; n < arr.ndim; n++) {
+        asm volatile ("");
         num *= arr.dims[n];
     }
     return num;
}

How should we fix this in general? The optimization may be preventable using a better method but I'm not sure of one. This optimization "bug" is not present in GCC 4.4 (CentOS 6) or GCC 4.7+

[Joseph Naegele]

Here's a sensible change that prevents the bad optimization in GCC 4.6.3-1ubuntu5:

template <typename T> size_t NDArray<T>::getNumberOfElements() {
     size_t num = 1;
     for (int n = 0; n < arr.ndim; n++) {

-        num *= arr.dims[n];
+        size_t v = arr.dims[n];
+        if (v > 0) {
+            num *= v;
+        }
     }
     return num;
 }

[Michael Hansen]

Seems ok to me. 

[Ghislain Vaillant]

The PPA has been updated. It should show up in the updates soon.

Thanks again Jo for chasing down the bug and pushing the fix.

Ghis