Quantcast

[gdal-dev] Resizing CPLErrorContext?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[gdal-dev] Resizing CPLErrorContext?

Kurt Schwehr-2
w.r.t. https://trac.osgeo.org/gdal/changeset/38405

Exactly why is it okay to resize this fixed size structure?

typedef struct {
    CPLErrorNum nLastErrNo;
    CPLErr  eLastErrType;
    CPLErrorHandlerNode *psHandlerStack;
    int     nLastErrMsgMax;
    int     nFailureIntoWarning;
    char    szLastErrMsg[DEFAULT_LAST_ERR_MSG_SIZE];
    // Do not add anything here. szLastErrMsg must be the last field.
    // See CPLRealloc() below.
} CPLErrorContext;

Your comment is:

CPLErrorSetState(): Workaround clang -fsanitize=undefined behaviour that doesn't like dereferencing szLastErrMsg[i>=DEFAULT_LAST_ERR_MSG_SIZE] even when structure has been properly resize. Fixes ​https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=1628. Credit to OSS Fuz

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Resizing CPLErrorContext?

Even Rouault-2

On mardi 16 mai 2017 07:01:53 CEST Kurt Schwehr wrote:

> w.r.t. https://trac.osgeo.org/gdal/changeset/38405

>

> Exactly why is it okay to resize this fixed size structure?

 

Well, this is a good old trick of grey-beard C programmers, isn't it ? Not sure what the C/C++ standards says about that, but it works pretty well in practice.

 

Some explanations at:

http://stackoverflow.com/a/3123792

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Resizing CPLErrorContext?

Kurt Schwehr-2
I now know this is referred to as the "struct hack."

This really is undefined behavior in the C++11 standard...  there was a proposal to make the struct hack valid for C++11:


From another google engineer:

5.2.1 tells you that x[y] is identical to *(x + y). 
5.7.5 tells you that x + y has undefined behavior if the result points outside of the array.

From the C++11 standard:

5.2.1 Subscripting [expr.sub] 
1 A postfix expression followed by an expression in square brackets is a postfix expression. One of the expressions 
shall have the type “pointer to T” and the other shall have unscoped enumeration or integral type. 
The result is an lvalue of type “T.” The type “T” shall be a completely-defined object type.62 The expression 
E1[E2] is identical (by definition) to *((E1)+(E2)) [ Note: see 5.3 and 5.7 for details of * and + and 8.3.4 
for details of arrays. — end note ]

5.7 Additive operators 
...

5.7.5 When an expression that has integral type is added to or subtracted from a pointer, the result has the type 
of the pointer operand. If the pointer operand points to an element of an array object, and the array is 
large enough, the result points to an element offset from the original element such that the difference of 
the subscripts of the resulting and original array elements equals the integral expression. In other words, if 
the expression P points to the i-th element of an array object, the expressions (P)+N (equivalently, N+(P)) 
and (P)-N (where N has the value n) point to, respectively, the i + n-th and i − n-th elements of the array 
object, provided they exist. Moreover, if the expression P points to the last element of an array object, 
the expression (P)+1 points one past the last element of the array object, and if the expression Q points 
one past the last element of an array object, the expression (Q)-1 points to the last element of the array 
object. If both the pointer operand and the result point to elements of the same array object, or one past 
the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is 
undefined.



On Tue, May 16, 2017 at 7:35 AM, Even Rouault <[hidden email]> wrote:

On mardi 16 mai 2017 07:01:53 CEST Kurt Schwehr wrote:

> w.r.t. https://trac.osgeo.org/gdal/changeset/38405

>

> Exactly why is it okay to resize this fixed size structure?

 

Well, this is a good old trick of grey-beard C programmers, isn't it ? Not sure what the C/C++ standards says about that, but it works pretty well in practice.

 

Some explanations at:

http://stackoverflow.com/a/3123792

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com





_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Resizing CPLErrorContext?

Even Rouault-2

On mardi 16 mai 2017 10:26:09 CEST Kurt Schwehr wrote:

> I now know this is referred to as the "struct hack."

>

> This really is undefined behavior in the C++11 standard...

 

Well, I don't think the hack is really needed and char szLastErrorMsg[] could be replaces by a char* pszLastErrorMsg that would be malloc'ated.

The current situation comes from the fact that originally no custom free function could be associated with a TLS member, but now that we have a

CPLSetTLSWithFreeFunc() we could use it to free the pszLastErrorMsg member before freeing the context.

 

(or use std::string if you prefer, but this code portion can be run under very low memory conditions, so great care is needed)

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Resizing CPLErrorContext?

Kurt Schwehr-2
I would strongly lean towards std::string if possible.

I'm not sure how a realloc on the heap is any less safe than a std::string and strings can have memory reserved at construction time (or ::reserve()).  Or in crazy cases, a custom allocator can be used that has a preallocated pool of memory available for when the system is OOM. 

A related question... any idea what the '0' char is for?  It looks like you introduced the zero char after \n in https://trac.osgeo.org/gdal/changeset/14601  and I don't see '0' in https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_error.cpp?annotate=blame&rev=14600

On Tue, May 16, 2017 at 11:01 AM, Even Rouault <[hidden email]> wrote:

On mardi 16 mai 2017 10:26:09 CEST Kurt Schwehr wrote:

> I now know this is referred to as the "struct hack."

>

> This really is undefined behavior in the C++11 standard...

 

Well, I don't think the hack is really needed and char szLastErrorMsg[] could be replaces by a char* pszLastErrorMsg that would be malloc'ated.

The current situation comes from the fact that originally no custom free function could be associated with a TLS member, but now that we have a

CPLSetTLSWithFreeFunc() we could use it to free the pszLastErrorMsg member before freeing the context.

 

(or use std::string if you prefer, but this code portion can be run under very low memory conditions, so great care is needed)

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Resizing CPLErrorContext?

Even Rouault-2

>

> A related question... any idea what the '0' char is for? It looks like you

> introduced the zero char after \n in

> https://trac.osgeo.org/gdal/changeset/14601 and I don't see '0' in

> https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_error.cpp?annotate=b

> lame&rev=14600

 

Should have been '\0' indeed. Fixed. This has no practical consequence since this character is just overwritten by the CPLvsnprintf() call a few lines after.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Loading...