1

I'm having difficulty using malloc in a function where I read a binary file with 4 byte unsigned integers, free the passed array reference, remalloc it to the new size and then try to access members of the array. I think the problem is due to the uint32_t type as the array seems to be being seen as a array of 8 byte integers rather than 4 byte ones. Not exactly sure where I am going wrong, it might be with the use of malloc, IE maybe I need to instruct it to create uint32_t types in a different way to what I am doing, or maybe its something else. Code:

#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <unistd.h>
#include <string.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <malloc.h>

int filecheck (uint32_t **sched, int * count) {
    int v, size = 0;
    FILE *f = fopen("/home/pi/schedule/default", "rb");
    if (f == NULL) { 
        return 0; 
    } 
    fseek(f, 0, SEEK_END);
    size = ftell(f);
    fseek(f, 0, SEEK_SET);
    int schedsize = sizeof(uint32_t);
    int elementcount = size / schedsize;
    free(*sched);
    *sched = malloc(size);
    if (elementcount != fread(sched, schedsize, elementcount, f)) { 
        free(*sched);
        return 0;
    } 
    fclose(f);
// This works correctly and prints data as expected
    for (v=0;v<elementcount;v++) { 
        printf("Method 1 %02d %u \n", v, ((uint32_t*)sched)[v]);
    }

// This skips every other byte byt does not print any numbers > 32 bit unsigned
    for (v=0;v<elementcount;v++) { 
        printf("Method 2 %02d %u \n", v, sched[v]);
    }
// This treats the binary file as if it was 64 bit uints, printing 64 bit numbers
    for (v=0;v<elementcount;v++) { 
        printf("Method 3 %02d %lu \n", v, sched[v]);
    }
    *count = elementcount;
    return 1;
}

int main (){
    uint32_t *sched = NULL;
    int i, count = 0;
    if (filecheck(&sched, &count)) {
        for (i=0;i<count;i++) { // At the next line I get a segmentation fault
            printf("Method 4 %02d %u\n", i, sched[i]);
        }
    } else {
        printf("Error\n");
    }
    return 0;
}

I added the file reading code as requested. Trying to access the array sched in main() the way I am doing will print the data I am reading as if it was 8 byte integers. So I guess sched is being seen as an array of 64 bit integers rather than 32 bit as I have defined it as. I guess this is because I freed it and used malloc on it again. But I believe I instructed malloc that the type it should create is a uint32_t so I am confused as to why the data is not being treated as such.

Edit: Found a way to make it work, but not sure if its right (method 1). Is this the only and cleanest way to get this array treated as uint32_t type? Surely the compiler should know what type I am dealing with without me having to cast it every time I use it.

Edit: Actually when I try to access it in main, I get a seg fault. I added a comment in the code to reflect that. I didnt notice this before as I was using the for print loop in several places to trace how the data was being viewed by the compiler.

Edit: Not sure if there is a way I can add the binary data file. I am making it by hand in a hex editor and its exact content is not that important. In the bin file, FF FF FF FF FF FF FF FF should be read as 2 uint32_t types, rather than as 1 64 bit integer. Fread AFAIK does not care about this, it just fills the buffer, but stand to be corrected if thats wrong.

TIA, Pete

Pete
  • 335
  • 1
  • 3
  • 12
  • 2
    You ask about data you read, without showing how you read it, and why it displays the way it does, without showing what that output was. Besides the cast malloc ([which you shouldn't do in C)](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc?s=1|7.0476), Your allocation calls appear correct for storing 10x `uint32_t`. The only thing off is the `printf` specifier, which should be formed via concatenation from the appropriate constant in `inttypes.h` when using `uint32_t` But *how* you're loading you're array and *what* you're loading it with is all on you. – WhozCraig May 08 '15 at 03:55
  • To print a `uint32_t` correctly, do something like this: `printf("%"PRIu32"\n", x);` (that requires `inttypes.h`, of course). – Iskar Jarak May 08 '15 at 03:58
  • Sorry - I will get to work shaving down the file reading bits and that will fill in the gaps I suppose. But to cut to the chase, its just that the data is not being printed correctly, its being indexed incorrectly too. I've played with lots of printf specifiers, and although some create different results than others, they all either skip every other byte, or view the data as 64 bit bytes. The indexing of the array is wrong. The pointer is being treated as a pointer to a 64 bit type, not a 32 bit type. – Pete May 08 '15 at 04:02
  • @Pete what does `printf("%zu\n", sizeof(uint32_t));` say? – Iskar Jarak May 08 '15 at 04:20
  • @Iskar, it says 4, which I interpret as meaning 4 byte words, ie 32 bit. If I make sched an array and assign values to it thus (uint32_t sched[4] = {0,1,2,3};) then accessing it array stule works. Its just when I use malloc that the type seems to become int (64 bit on my machine). Despite trying to tell the compiler that its meant to be a uint32_t. So I wonder if I am not doing that bit right. – Pete May 08 '15 at 04:26
  • 1
    @WhozCraig thanks re the cast malloc, have read that link and will adjust accordingly. – Pete May 08 '15 at 04:31
  • @Pete Your `fread` into `sched` instead of `*sched` is probably the cause of the segfault when you loop. See my answer. – Iskar Jarak May 08 '15 at 05:01

1 Answers1

1
// This works correctly and prints data as expected
    for (v=0;v<elementcount;v++) { 
        printf("Method 1 %02d %u \n", v, ((uint32_t*)sched)[v]);
    }

This is not quite right, because sched is a uint32_t**, and you allocated to *sched. And also because %u is not how you should print uint32_t. First dereference sched and then apply the array index access.

You want printf("%02d %"PRIu32"\n", v, (*sched)[v]));

// This skips every other byte byt does not print any numbers > 32 bit unsigned
    for (v=0;v<elementcount;v++) { 
        printf("Method 2 %02d %u \n", v, sched[v]);
    }

Yeah, that's because sched[v] is a uint32_t*, and since it's a pointer type and you're probably running on a 64-bit machine, it's probably 64 bits... so you're iterating with 8-byte increments instead of 4-byte ones and trying to print pointers as %u. It also goes out of bounds because 8*10 is larger than 4*10, and that is likely to cause a segmentation fault.

// This treats the binary file as if it was 64 bit uints, printing 64 bit numbers
    for (v=0;v<elementcount;v++) { 
        printf("Method 2 %02d %lu \n", v, sched[v]);
    }

This is like the second example only you're printing with the marginally more appropriate but still incorrect %lu.

You need to fread() into *sched instead of sched, as well, or you will cause UB and this will very likely cause a segmentation fault when you try to read the array.

Also, your for loop in main should never run since you aren't setting count to anything in filecheck so it should still be zero (in the absence of UB, at least).

Other comments:

  • As you've just learned, don't cast the result of malloc() in C.
  • main() should return a value. 0 if everything went fine.
  • You can write some uint32_t to a file as bytes using fwrite(), of course.
  • Check the result of malloc() to see if it succeeded.
  • Use size_t for size_t values instead of int, and print accordingly (%zu).
Iskar Jarak
  • 5,136
  • 4
  • 38
  • 60
  • Ah! Was playing round with some of your suggestions but it still wasn’t working but then fread into *sched as you said and then it came together. After working through some other posts and tutorials I think I understand properly too. Could you expand on the last comment you made, re using size_t? Also, is including inttypes.h the only way to printf these uint32_t's? Thanks, Pete – Pete May 08 '15 at 05:53
  • @Pete Yes, as far as I know, including `inttypes.h` is the only portable way to print `uint32_t` and co. Things like `fread()` take and return `size_t`, which is a special unsigned integer type guaranteed to be able to hold any legal size. While in this case, it's no big deal, if you were dealing with really big arrays this could catch you out. Implicit (automatic) casting `size_t` to/from `int` could also catch you out if you don't realise that it's happening. In the same vein, `ftell` returns a `long` - another potential gotcha if you're storing the result as `int`. – Iskar Jarak May 08 '15 at 06:01
  • @Pete Basically, just be aware of what types are expected or returned by the stdlib/system calls you're using and one day you might avoid a horrible bug. It matters particularly for writing portable code that will run on platforms with different type sizes. And if you think my answer helped, the way SO works is to consider [accepting it](http://stackoverflow.com/help/accepted-answer) – Iskar Jarak May 08 '15 at 06:02
  • Absolutely Iskar, accepted right now, thanks very much too. Also for the extra info re the sizes, regarding ftell and such - I didn't really know about this and it points me towards the reading I need to do to make sure this simple little program is bug free which it needs to be as its long running on low powered hardware. Great answer, got me motoring again, plus extra info. Will plus one if I can too. – Pete May 08 '15 at 07:12