Application Crash and Faulting Offset

[SGaist]

@Juan-Dev said in Application Crash and Faulting Offset:

In my case the use of pointers is done with unsigned char* ptr to manage a block of memory.
A function is provided to me allowing me to allocate memory, I just have to pass it the address of a pointer and the desired memory size.

@Juan-Dev, in addition to the @JonB's request, can you share the code that is related to that as well ? And if possible, tell us where that function comes from ?

[Juan Dev]

@JonB said in Application Crash and Faulting Offset:

I suggest you post a screenshot or paste something of precisely what you see when it does "crash"? (Not what you come across in Event Viewer.) What are you seeing which even tells you your program has "crashed", you have not answered this?

My application closes by itself. And I get the message (when I'm on QT) :
..\..\MSVC2019_64bit-Release\release\application_name.exe crashed.

[SimonSchroeder]

@Juan-Dev said in Application Crash and Faulting Offset:

// I should declare it like this...?
unsigned char* ptr = nullptr;

It is good practice to always initialize variables when you are declaring them. This can avoid a lot of problems. There are only very few cases where this is not (easily) possible. Sometimes it means declaring your variable a little later when all information is available. Bonus tip: mark as many variables as const as you can.

[Juan Dev]

First of all, thank you for all your response and your time spent.
In order to move forward as best as possible, I continued to search to find in the long code that this program composes, where the problem could arise.
I located the problem function and then I placed a return in different places in this function.

With the code below (an extract of the code in fact) my program runs correctly and the function exits correctly

unsigned char* contenuCrlFse = nullptr;
contenuCrlFse = (unsigned char*)malloc(SSV_LONG_CRL_FSE + 1 * sizeof(char));

/*
...
Here we find code that retrieves the content of contenuCrlFse
...
*/

qDebug() << "Before Clean Exit of the Function";
free(contenuCrlFse); return EXIT_SUCCESS;

// Finalization
contenuCrlFse[SSV_LONG_CRL_FSE] = '\0';

With the code below (an extract of the code in fact) the function exit does not take place

unsigned char* contenuCrlFse = nullptr;
contenuCrlFse = (unsigned char*)malloc(SSV_LONG_CRL_FSE + 1 * sizeof(char));

/*
...
Here we find code that retrieves the content of contenuCrlFse
...
*/

// Finalization
contenuCrlFse[SSV_LONG_CRL_FSE] = '\0';

qDebug() << "Before Clean Exit of the Function";
free(contenuCrlFse); return EXIT_SUCCESS;

And I had an error (in Debug mode) which appears and which is the following

And these two snippets use the constant below
#define SSV_LONG_CRL_FSE 40

[JonB]

@Juan-Dev
FWIW, with the code as shown, and nothing else (nothing in the commented out Here we find code that retrieves the content of contenuCrlFse section) should not generate the "buffer overrun" error. Have you tested it all on its own and in isolation from anything else? What else you might have from which this is an "extract" I cannot say..

You might output the hex value of contenuCrlFse pointer to compare against the address in the error message.

[Juan Dev]

With the help of your various comments I continued to look to find out where the problem came from and it certainly comes from the extraction. "code that retrieves the content".
I recreated a code snippet. Initially my code was as follows :
main.cpp

// Initialization
unsigned char* fullContent = nullptr;
size_t sizeFullContent = 0;
unsigned char* extractedContent = nullptr;
size_t sizeExtractedContent = 0;
int ret = 0;

// Get Full Content
ret = getFullContent(&fullContent,&sizeFullContent);  if(ret!=EXIT_SUCCESS) return EXIT_FAILURE;

// Memory Allocation
extractedContent = (unsigned char*)malloc(LONG_EXTRACT + 1 * sizeof(char));

// Content Extraction
ret = extractContent(fullContent,sizeFullContent,150,&extractedContent,&sizeExtractedContent);

// Extraction control
if(ret!=EXIT_SUCCESS) return EXIT_FAILURE;
if(sizeExtractedContent!=LONG_EXTRACT) return EXIT_FAILURE;

// Finalization
extractedContent[LONG_EXTRACT] = '\0';

functions.cpp

int extractContent(unsigned char* ptrMemoryFonc, size_t sizeMemoryFonc, int numFiled, unsigned char ** ptrExtractFonc, size_t * sizeExtractFonc)
{
  unsigned char* ptrExtract = nullptr;
  size_t sizeExtract = 0;
  size_t cursorStart = 0;

  // Here I have code to iterate through memory "ptrMemoryFonc" looking for "numField" and I get "cursorStart" and "sizeExtract"
  cursorStart = 21;
  sizeExtract = 10;

  // Memory Allocation
  ptrExtract = (unsigned char*)malloc(sizeExtract * sizeof(unsigned char));	if (ptrExtract == NULL) return EXIT_FAILURE;

  // For Each Character in the Field Found - Memorization
  for (size_t i = cursorStart; i < cursorStart + sizeExtract; i++)	ptrExtract[i - cursorStart] = ((unsigned char*)ptrMemoryFonc)[i];

  // Memorization
  *ptrExtractFonc = ptrExtract;
  *sizeExtractFonc = sizeExtract;

  return EXIT_SUCCESS;
}

In this initial code, I made the mistake of not freeing memory with free(extractedContent) in main.cpp
And when I wanted to free the memory that's when it generated an error.
So I reviewed my code and it is now the following :
main.cpp

// Initialization
unsigned char* fullContent = nullptr;
size_t sizeFullContent = 0;
unsigned char* extractedContent = nullptr;
size_t sizeExtractedContent = 0;
int ret = 0;

// Get Full Content
ret = getFullContent(&fullContent,&sizeFullContent);  if(ret!=EXIT_SUCCESS) return EXIT_FAILURE;

// Content Extraction
ret = extractContent(fullContent,sizeFullContent,150,&extractedContent,&sizeExtractedContent);

// Extraction control
if(ret!=EXIT_SUCCESS) return EXIT_FAILURE;
if(sizeExtractedContent!=LONG_EXTRACT) return EXIT_FAILURE;

// Finalization
extractedContent[LONG_EXTRACT] = '\0';

// Memory Release
free(extractedContent);

functions.cpp

int extractContent(unsigned char* ptrMemoryFonc, size_t sizeMemoryFonc, int numFiled, unsigned char ** ptrExtractFonc, size_t * sizeExtractFonc)
{
  unsigned char* ptrExtract = nullptr;
  size_t sizeExtract = 0;
  size_t cursorStart = 0;

  // Here I have code to iterate through memory "ptrMemoryFonc" looking for "numField" and I get "cursorStart" and "sizeExtract"
  cursorStart = 21;
  sizeExtract = 10;

  // Memory Allocation
  ptrExtract = (unsigned char*)malloc(sizeExtract+1 * sizeof(unsigned char));	if (ptrExtract == NULL) return EXIT_FAILURE;

  // For Each Character in the Field Found - Memorization
  for (size_t i = cursorStart; i < cursorStart + sizeExtract; i++)	ptrExtract[i - cursorStart] = ((unsigned char*)ptrMemoryFonc)[i];

  // Memorization
  *ptrExtractFonc = ptrExtract;
  *sizeExtractFonc = sizeExtract;

  return EXIT_SUCCESS;
}

What I changed :

• In the "main.cpp" I no longer allocate memory with malloc for "extractedContent"
• It is in the function that I allocate memory (adding 1 in anticipation of the '\0' character which will be added later)

But is the code correct...? Is my content extraction function correct...?

[JonB]

@Juan-Dev
Yes, it looks like in the old code you did not allocate room for the extra byte for ptrExtract. Now you do.

Since it is extractContent() which allocates room for the terminating \0 byte I would set that byte in extractContent() rather than in main.cpp for clarity, but that is up to you.

You code currently relies on the LONG_EXTRACT in main.cpp being equal to the sizeExtract calculated(?) in extractContent(). It could be less than that, but must not be more (because of the malloc()). This is the kind of "hidden" requirement which can be hard to spot if it goes wrong, you should link these two values as appropriate.

In case you are not aware, you might like to use std::memcpy( void* dest, const void* src, std::size_t count ) (or C memcpy()) to copy the bytes instead of your for loop:

std::memcpy(ptrExtract, ptrMemoryFonc + cursorStart, sizeExtract);

Less of your own code to check/clearer :)

[Juan Dev]

@JonB Thank for your answer

Since it is extractContent() which allocates room for the terminating \0 byte I would set that byte in extractContent() rather than in main.cpp for clarity, but that is up to you.

The extract function is sometimes used to extract an area to which I do not necessarily add a '\0' character. This is why I did not integrate this addition into the function itself.
But we agree that even if this character will not be added later, I can reserve a memory space with one more character in my function, this does not pose a problem...?

In case you are not aware, you might like to use std::memcpy() to copy the bytes instead of your for loop.
Less of your own code to check/clearer :)

We agree on the code to check :). Thank you for this valuable information. I'll look into setting this up.

[JonB]

@Juan-Dev said in Application Crash and Faulting Offset:

But we agree that even if this character will not be added later, I can reserve a memory space with one more character in my function, this does not pose a problem...?

Absolutely, this is fine. And you must do so in case you do add the terminator. So far as the malloc()/free() is concerned that works fine; what you do with/without the terminating \0 is a different matter.

You still have a "dependency" between the value of LONG_EXTRACT in the caller and the value of sizeExtract in extractContent()'s call to malloc(). If LONG_EXTRACT > sizeExtract you will have a "hidden" write of a byte beyond the malloc()ed area, which can be hard to spot. I would still make some relationship between those two values, even if it's just that the caller checks LONG_EXTRACT against the returned &sizeExtractedContent. If they are supposed to be the same extractedContent[sizeExtractedContent] = '\0'; would be safer. Using a separate LONG_EXTRACT here only makes sense if it can be less than sizeExtractedContent.

[Juan Dev]

@JonB
The part of the code I posted is confusing, I'm sorry.
But I don't always have a dependency in the full application code.

In the full application, the extractContent() function can be used to extract variable-length portions of memory.
And for some extracted portions I control the fixed length and add a '\0' character.
And for others I simply use the extracted portion without checking or adding characters