1

While freeing some pointers, I get an access violation.
In order to know what's going on, I've decided to ask to free the pointers at an earlier stage in the code, even directly after memory has been allocated, and still it crashes.
It means that something is seriously wrong in the way my structures are handled in memory.

I know that in a previous version of the code, there was a keyword before the definition of some variables, but that keyword is lost (it was part of a #define clause I can't find back).

Does anybody know what's wrong in this piece of code or what the mentioned keyword should be?

typedef unsigned long longword;
typedef struct part_tag { struct part_tag *next;
                                 __int64 fileptr;
                                 word needcount;
                                 byte loadflag,lock;
                                 byte partdat[8192];
                                 } part;

static longword *partptrs;

<keyword> part *freepart;
<keyword> part *firstpart;

void alloc_parts (void) {
  part *ps;
  int i;

  partptrs = (longword*)malloc (number_of_parts * sizeof(longword)); // number... = 50
  ps = (part*)&freepart;

  for (i=0; i<number_of_parts; i++) {
    ps->next = (struct part_tag*)malloc(sizeof(part));
    partptrs[i] = (longword)ps->next;
    ps = ps->next;
    ps->fileptr = 0; ps->loadflag = 0; ps->lock = 0; ps->needcount = 0; // fill in "ps" structure
  };
  ps->next = nil;
  firstpart = nil;
  for (i=0; i<number_of_parts; i++) {
    ps = (part*)partptrs[i];
    free(ps); <-- here it already crashes at the first occurence (i=0)
  };

}

Thanks in advance

In the comments somebody asks why I'm freeing pointers directly after allocating them. This is not how the program originally was written, but in order to know what's causing the access violation I've rewritten in that style.
Originally:

alloc_parts();
<do the whole processing>
free_parts();

In order to analyse the access violation I've adapted the alloc_parts() function into the source code excerpt I've written there. The point is that even directly after allocating memory, the freeing is going wrong. How is that even possible?

In the meanwhile I've observed another weird phenomena:
While allocating the memory, the values of ps seem to be "complete" address values. While trying to free the memory, the values of ps only contain the last digits of the memory addresses.

Example of complete address :        0x00000216eeed6150
Example of address in freeing loop : 0x00000000eeed6150 // terminating digits are equal,
                                                        // so at least something is right :-)

This problem was caused by the longword type: it seems that this type was too small to hold entire memory addresses. I've replaced this by another type (unsigned long long) but the problem still persists.

Dominique
  • 16,450
  • 15
  • 56
  • 112
  • What is `longword`? Please [post a *complete* example.](http://stackoverflow.com/help/mcve) – Andrew Henle Feb 14 '17 at 13:09
  • What is `longword`? Why are you allocating `number_of_parts * longwords` bytes instead of e.g. `number_of_parts * sizeof(part)`? And most importantly, why are you using `&freepart` which will give you a pointer *to the pointer*? – Some programmer dude Feb 14 '17 at 13:09
  • 1
    This is very bad code, it treats pointers as integers. If those have different sizes, you're in for a treat. Rewrite it without the strange `longword` business. – unwind Feb 14 '17 at 13:10
  • 2
    `ps = (part*)&freepart;` is bad. – BLUEPIXY Feb 14 '17 at 13:10
  • I've added the `typedef longword` I'd forgotten. As for the other comments: I didn't write this piece of code, I inherited it from a collegue who has inherited it ... (you know how it goes :-) ). I'd like to modify this existing code as less as possible, but still get rid of that access violation. – Dominique Feb 14 '17 at 13:13
  • @Dominique: did your colleague put these explicit pointer-to-integer casts in order to shut the "stupid" compiler warnings off? – Blagovest Buyukliev Feb 14 '17 at 13:16
  • @BlagovestBuyukliev: that's very well possible :-) – Dominique Feb 14 '17 at 13:19
  • @BLUEPIXY: By what should I replace that line `ps = (part*) &freepart;`? – Dominique Feb 14 '17 at 13:21
  • 2
    freepart is not pointing anywhere so your first _ps->next =_ is writing to nowhere land. freepart should be a part: not a part* – cup Feb 14 '17 at 13:24
  • @Dominique I think that it is necessary to rewrite the whole as well as that part. – BLUEPIXY Feb 14 '17 at 13:26
  • The code allocs `part`s and then immediately frees them again? But.... WHY? – Klas Lindbäck Feb 14 '17 at 13:27
  • 2
    might be easier and quicker to just rewrite the whole function from scratch - minus all that `longword` stuff – Chris Turner Feb 14 '17 at 13:28
  • `ps = (part*)partptrs[i]; free(ps);` It is not possible to release a part secured by `malloc`. – BLUEPIXY Feb 14 '17 at 13:28
  • @BLUEPIXY : not possible to free a pointer, secured by ´malloc()´? Never heard of this. What do you mean? – Dominique Feb 14 '17 at 13:31
  • 1
    E.g `Type *p = malloc(n * sizeof(Type));` , It can `free(p);`, cannot `free(&p[i]);` – BLUEPIXY Feb 14 '17 at 13:33
  • 1
    @BLUEPIXY There is a loop that mallocs `partptrs[i]` (via `ps->next`). – Klas Lindbäck Feb 14 '17 at 14:00
  • @KlasLindbäck Ah, yes. There are lots of complex errors.:D – BLUEPIXY Feb 14 '17 at 14:04
  • 3
    @Dominique Try not to inherit any more code from this colleague. You'll do better by deleting the entire mess and starting from nothing. Figure out what the code is supposed to do, and do *that*. Trying to stuff a pointer value into a `long` is utterly brain damaged - for example, on 64-bit Windows it doesn't even fit. – Andrew Henle Feb 14 '17 at 14:05
  • Sorry for the long absence but I've been assigned to other, more urgent, tasks. I'm back at this task now and I've understood that this issue has started immediately after going from Visual Studio 2010 to Visual Studio 2013. In the previous versions of the source code, there were some keywords like `own` and `local` (which I don't know and don't find back anymore), I believe they meant something like `static` but using that keyword doesn't solve the issue. – Dominique Mar 07 '17 at 12:58
  • @BLUEPIXY You mention that it's impossible to free something secured by `malloc`, can you tell me by what I should replace the line `free(ps)` in order to get this working? I'm afraid that just deleting this piece of code might result in a memory leak. Please write your response as an answer, when correct I'll accept your answer. – Dominique Mar 07 '17 at 14:32
  • Start by removing the cast first. – BLUEPIXY Mar 07 '17 at 15:03
  • @BLUEPIXY: I've just tried to remove the cast, but unfortunately the resulting code only works for the first entry of the array: `void free_parts (void) { longword* ps; for (int i = 0 ; i < number_of_parts ; i++) { ps = &partptrs[i]; free (ps); } } ` – Dominique Mar 07 '17 at 15:43
  • [Don't cast the result of `malloc` in C](http://stackoverflow.com/q/605845/995714) – phuclv Apr 19 '17 at 12:05

1 Answers1

0

Finally, after a long time of misery, the problem is solved:

The program was originally meant as a 32-bit application, which means that the original type unsigned long was sufficient to keep memory addresses.

However, this program gets compiled now as a 64-bit application, hence the mentioned type is not sufficiently large anymore to keep 64-bit memory addresses, hence another type has been used for solving this issue:

typedef intptr_t longword;

This solves the issue.

@Andrew Henle: sorry, I didn't realise that your comment contained the actual solution to this problem.

Dominique
  • 16,450
  • 15
  • 56
  • 112