libclamav/vba_extract.c: Fix SIGBUS on HP-UX/IA when built as a 64-bit binary

[twwlogin]

Fix for issue #525.

[shutton]

This doesn't strike me as the best way to fix this (and might even break it). I think the problem really started upstream, in that cvi_vba_inflate() returns a char *, but that's a cast of blobToMem()'s return type, which is void *. Had the return chain simply remained consistent with void *, this could have been used in cli_vba_readdir_new() rather than char *, which your compiler seems to be treating more liberally with respect to alignment.

I'm also concerned that this is breaking indexing into data since the expectation is that it can be indexed byte-wise, but declaring it as a larger integer will cause indexing to be at that granularity.

Bottom line, I think the better fix is to simply declare it void *, and then alias to (char *) so that it's correctly aligned.

[twwlogin]

Bottom line, I think the better fix is to simply declare it void *, and then alias to (char *) so that it's correctly aligned.

So your suggestion is something like:
--- vba_extract.c.orig 2022-04-03 13:00:41 +0000
+++ vba_extract.c 2022-04-05 21:27:27 +0000
@@ -363,7 +363,7 @@
cl_error_t ret = CL_SUCCESS;
char fullname[1024];
int fd = -1;

• unsigned char *data = NULL;

• void *data = NULL;
  size_t data_len;
  size_t data_offset;
  const char *stream_name = NULL;

[shutton]

Bottom line, I think the better fix is to simply declare it void *, and then alias to (char *) so that it's correctly aligned.

So your suggestion is something like: --- vba_extract.c.orig 2022-04-03 13:00:41 +0000 +++ vba_extract.c 2022-04-05 21:27:27 +0000 @@ -363,7 +363,7 @@ cl_error_t ret = CL_SUCCESS; char fullname[1024]; int fd = -1;

• unsigned char *data = NULL;

• void *data = NULL;
  size_t data_len;
  size_t data_offset;
  const char *stream_name = NULL;

You'd need to test it on your platform to confirm it resolves the error (since we definitely don't have any HP-UX/IA systems in our test pipelines), but I was thinking something along these lines (to minimize the changed code):

void *inflate_data = NULL
char *data = NULL;
...
    if ((inflate_data = (data_type_t *)cli_vba_inflate(fd, 0, &data_len)) == NULL) {
         cli_dbgmsg("vba_readdir_new: Failed to decompress 'dir'\n");
         ret = CL_EARG;
         goto done;
    }
    data = inflate_data;
...

I'm thinking that will coax a compatible alignment without additional trickery.

[twwlogin]

void *inflate_data = NULL
char *data = NULL;
...
    if ((inflate_data = (data_type_t *)cli_vba_inflate(fd, 0, &data_len)) == NULL) {
         cli_dbgmsg("vba_readdir_new: Failed to decompress 'dir'\n");
         ret = CL_EARG;
         goto done;
    }
    data = inflate_data;
...

Did you mean to leave the cast to (data_type_t *)?

[twwlogin]

BTW, I compiled vba_extract.c with -Wcast-align on this platform and it gives a warning on any call to le16_to_host() and le32_to_host() where &data[data_offset] is referenced:

orig-vba_extract.c: In function 'cli_vba_readdir_new':
orig-vba_extract.c:474:14: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:474:14: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:482:16: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:482:16: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:482:16: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:482:16: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:499:37: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:499:37: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:499:37: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:499:37: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:535:33: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:535:33: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:535:33: warning: cast increases required alignment of target type [-Wcast-align]
orig-vba_extract.c:535:33: warning: cast increases required alignment of target type [-Wcast-align]
...

[twwlogin]

How about something like this as a solution. Rather than:

size = le32_to_host(*(uint32_t *)&data[data_offset]);

we would use:

uint32_t val;
memcpy(&data[data_offset], &val, sizeof (uint32_t));
size = le32_to_host(val);

Less efficient though.

[shutton]

(Comment based on the current PR content)

Doesn't changing the type alter how this would work?

&data[data_offset]

You'd need to adjust all those macros to adjust the index value.

E.g., take this example code:

#include <stdio.h>
#include <stdint.h>

int main() {
    unsigned char *stuff1 = NULL;
    printf("stuff1[1] = %p\n", &stuff1[1]);
    uint32_t  *stuff2 = NULL;
    printf("stuff2[1] = %p\n", &stuff2[1]);
}

You get rather different results:

stuff1[1] = 0x1
stuff2[1] = 0x4

[twwlogin]

We switch to using memcpy() as illustrated in this commit: 2098a84. Hopefully has less issues.

[micahsnyder]

Reading over this code the latest iteration seems okay to me. I just rebased it with the latest from the main branch and squashed the commits into one.

If all goes well with the test pipelines I'll merge it.

Sorry this PR sat idle for so very long.

[micahsnyder]

Testing looked good. Only saw expected failures with. Nothing new or unusual.