Unaligned access in dap4

[pterjan]

After investigating some test failures in netcdf-c on arm due to a Bus Error in ncdump I found GETCOUNTER reads an unsigned long long which may not be properly aligned.

#0  0xf7f2e244 in delimitSeq (offsetp=<synthetic pointer>, vlentype=0xf7f2e2b8 <delimitSeqArray+208>, compiler=<optimized out>) at d4fix.c:388
        ret = 0
        i = <optimized out>
        offset = 0xf6e57016
        recordcount = <optimized out>
        recordtype = <optimized out>
        ret = <optimized out>
        i = <optimized out>
        offset = <optimized out>
        recordcount = <optimized out>
        recordtype = <optimized out>
        __PRETTY_FUNCTION__ = "delimitSeq"
        u = <optimized out>
        src = <optimized out>
#1  delimitSeqArray (compiler=0x7c798, varortype=<optimized out>, offsetp=0x0) at d4fix.c:368
        offset = 0xf6e57016
        i = 2220021216314720
        dimproduct = <optimized out>
        type = <optimized out>
#2  0xf7f2e940 in NCD4_delimit (compiler=compiler@entry=0x7c798, topvar=0x7dd50, offsetp=offsetp@entry=0xfffedf24) at d4fix.c:208
        ret = 0
        offset = <optimized out>
        __PRETTY_FUNCTION__ = "NCD4_delimit"
#3  0xf7f2f02c in NCD4_processdata (meta=0x7c798) at d4data.c:75
        var = <optimized out>
        ret = 0
        i = 0
        toplevel = 0x958c0
        root = 0xf7f2f02c <NCD4_processdata+128>
        offset = 0xf6e57008
#4  0xf7f2f9c0 in NCD4_open (path=<optimized out>, mode=<optimized out>, basepe=basepe@entry=0, chunksizehintp=chunksizehintp@entry=0x0, mpidata=0x0, mpidata@entry=0xf7ec06e0 <nc_open+32>,
    dispatch=dispatch@entry=0xf7fbadc4 <NCD4_dispatch_base>, ncid=65536) at d4file.c:237
        ret = 0
        d4info = 0x64ad8
        value = <optimized out>
        meta = 0x7c798
        nc = 0x64ab8
        __PRETTY_FUNCTION__ = "NCD4_open"
#5  0xf7ec05f4 in NC_open (path0=path0@entry=0x32c08 "[log][dap4]file:///home/iurt/rpmbuild/BUILD/netcdf-c-4.7.3/dap4_test/daptestfiles/test_sequence_2.syn", omode=<optimized out>, basepe=basepe@entry=0,
    chunksizehintp=chunksizehintp@entry=0x0, useparallel=useparallel@entry=0, parameters=parameters@entry=0x0, ncidp=0xfffee118, ncidp@entry=0xfffee110) at dfile.c:2117
        stat = <optimized out>
        ncp = 0x64ab8
        dispatcher = 0xf7fbadc4 <NCD4_dispatch_base>
        inmemory = <optimized out>
        diskless = <optimized out>
        mmap = <optimized out>
        path = 0x64f30 "file:///home/iurt/rpmbuild/BUILD/netcdf-c-4.7.3/dap4_test/daptestfiles/test_sequence_2.syn#mode=dap4&log"
        model = {impl = 6, format = 0}
        newpath = 0x64f30 "file:///home/iurt/rpmbuild/BUILD/netcdf-c-4.7.3/dap4_test/daptestfiles/test_sequence_2.syn#mode=dap4&log"

The crash happens when using GETCOUNTER as the offset is not 8 bytes aligned.

The following patch allowed the tests to pass:

--- netcdf-c-4.7.4/libdap4/ncd4.h	2020-03-27 15:33:36.000000000 +0000
+++ netcdf-c-4.7.4/libdap4/ncd4.h	2020-09-30 22:34:57.733422171 +0000
@@ -185,9 +185,15 @@
 #define DECR(offset,size) ((void*)(((char*)(offset))-(size)))
 #define DELTA(p1,p2) ((ptrdiff_t)(((char*)(p1))-((char*)(p2))))
 
+inline d4size_t __read_counter(void *p) {
+    COUNTERTYPE result;
+    memcpy(&result, p, sizeof(COUNTERTYPE));
+    return (d4size_t)result;
+}
+
 #undef GETCOUNTER
 #undef SKIPCOUNTER
-#define GETCOUNTER(p) ((d4size_t)*((COUNTERTYPE*)(p)))
+#define GETCOUNTER(p) __read_counter(p)
 #define SKIPCOUNTER(p) {p=INCR(p,COUNTERSIZE);}
 
 #undef PUSH

Given that all calls to GETCOUNTER seem to be followed by SKIPCOUNTER they may also be combined into a single function rather than two macros.

I also noticed many, but not all, calls to GETCOUNTER in d4fix.c then call if(compiler->swap) swapinline64(...); on the result, so there may also be some endianness bugs.

[WardF]

Thank you for reporting this. Out of curiosity, which ARM platform are you using?

[pterjan]

I am building it for armv7hl on Mageia, but we now do so on our aarch64 machines in which case Linux kernel does not recover unaligned accesses which is why I get Bus Error (If I was was building on an armv7 kernel it would catch the unaligned read and silently fix it https://www.kernel.org/doc/html/latest/arm/mem_alignment.html).

[DennisHeimbigner]

The problem in this case is that the binary data in the DAP4 response is not
necessarily aligned. It packs down to the byte level. Hence you can see attempts
to read unaligned data in all sorts of places in the DAP4 response. You just
happened to find this one. Fixing this is going to require a major change to the
DAP4 code to ensure that all reads from the response stream are copied into
an aligned location by memcpy or equivalent before decoding the value.

[pterjan]

I had a look around and didn't notice problems, it doesn't mean there is none but overall this seems well handled

In libdap4/d4data.c the data gets read with memcpy in all cases I saw

[DennisHeimbigner]

The dap4 algorithm is multipass. To oversimplify, it "compiles" pointers into the
response data packet in one pass. Then when extracting the data, it pulls it out.
It is the initial passe(s) that can read unaligned data.
In any case, I must confess surprise that there still exist machines that cannot
handle unaligned reads. Is this some kind of old hardware?

[pterjan]

Arm up to v7 requires aligned access, at minimum for 64 bis ones (LDRD which is the case in this bug) https://developer.arm.com/documentation/dui0473/m/using-the-assembler/address-alignment

Armv8 which is implemented by 64 bits arm cpus allows unaligned accesses https://developer.arm.com/architectures/learn-the-architecture/armv8-a-memory-model/alignment-and-endianness but is not yet that common

[DennisHeimbigner]

Well we can fix the DAP4 code contingent on running on Arm V7 or earlier.
Is there a way to detect this case?

[pterjan]

-Wcast-align=strict would give warnings even on other architectures in case of casting to a type requiring a larger alignment but there may be a lot of false positives depending on the code (if some aligned pointer get stored in a void*/char* and then casted again).

[DennisHeimbigner]

Ideally, I would have a compile time check so I could make the generated
code be contingent on alignment support. Since this probably impossible,
I guess the only solution is to add yet another ./configure/cmake option (ugh!).
Ward- do you have any thoughts on this?

[DennisHeimbigner]

Since this problem is restricted to 8-byte read -- or is write also a problem? --
An inline 8-byte move byte-by-byte might be faster than memcpy.

[DennisHeimbigner]

In looking around, I found these c-preprocessor flags:
defined(ARM_ARCH_7A)
defined(ARM_ARCH_7S)

So we can check for it, apparently. Which flag is appropriate or do I need
to check both?

[DennisHeimbigner]

Ok, I am attaching a zip file containing a modified version of
libdap4/ncd4.h. Please try it and see if it fixes your problem.
ncd4.zip

[DennisHeimbigner]

Have you had a chance to try my fix?

[DennisHeimbigner]

Fixed by #1871

[pterjan]

Sorry about the long delay. I haven't been able to test it properly yet as I got a different test failure when running the tests (which seems to be because hdf5 got updated to 1.10.7 on my distro 10 days ago causing an HDF Error in tst_nccopy4) and I didn't have much time to work on it.

However I manually tested the dap4 tests and the problem is still there. It seems here __ARM_ARCH_7A__ is defined but not __ARM_ARCH_7__

$ gcc -dM -E - < /dev/null | grep ARM_ARCH
#define __ARM_ARCH_ISA_ARM 1
#define __ARM_ARCH_PROFILE 65
#define __ARM_ARCH_ISA_THUMB 2
#define __ARM_ARCH 7
#define __ARM_ARCH_7A__ 1

The right condition may be something like defined(__arm__) && __ARM_ARCH < 8

[DennisHeimbigner]

Ok, I modified the PR to try your proposed condition.

[pterjan]

I am now getting an infinite loop somewhere and one test never finishes :(

Trying some change I noticed a warning: ncd4.h:192:24: warning: passing argument 1 of 'memcpy' makes pointer from integer without a cast [-Wint-conversion] which gave the reason, the argument of memcpy should be &v instead of v!

With this it now passes:

{COUNTERTYPE v; memcpy(&v,p,sizeof(v)); return (d4size_t)v;}