[gdal-dev] Grib2 Question

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Grib2 Question

Roarke Gaskill
Hi,

Why is the number of points greater than 33554432 considered nonsense?



Thanks,
Roarke



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

Re: Grib2 Question

Kurt Schwehr-2
It's possible to cause massive allocations with a tiny corrupted grib file causing an out-of-memory.  I found that case with the llvm ASAN fuzzer.  If you have a specification that gives a more reasoned maximum or a better overall check, I'm listening.  I definitely think the sanity checking can be improved.  Mostly I just try to survive the g2clib code.  It doesn't come with tests and digging through GRIB specs to match up to g2clib source isn't my favorite thing to do.


On Tue, Nov 7, 2017 at 1:22 PM, Roarke Gaskill <[hidden email]> wrote:
Hi,

Why is the number of points greater than 33554432 considered nonsense?



Thanks,
Roarke



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



--

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

Re: Grib2 Question

Even Rouault-2

On mardi 7 novembre 2017 13:51:30 CET Kurt Schwehr wrote:

> It's possible to cause massive allocations with a tiny corrupted grib file

> causing an out-of-memory. I found that case with the llvm ASAN fuzzer. If

> you have a specification that gives a more reasoned maximum or a better

> overall check, I'm listening. I definitely think the sanity checking can

> be improved. Mostly I just try to survive the g2clib code. It doesn't

> come with tests and digging through GRIB specs to match up to g2clib source

> isn't my favorite thing to do.

>

> https://github.com/OSGeo/gdal/commit/ae92f7fb8e32381124a37588d27b9af695afce2

> 0

 

I guess that if Roarke is asking the question he might have a dataset that breaks this limit ? If so, we might consider reverting that change, or making it more robust (which can be very tricky I know. Perhaps some heuristics with the file size ?), or just using it in fuzzing mode and not in production for now. And a pointer to such a dataset would be much appreciated.

 

(By the way: 2<<24 is IMHO an usual way of writing a limit. I confused it with 2^24 initially. So 1 << 25 would perhaps be better. Or just in decimal form as it is completely arbitary and not related to a binary encoding)

 

Even

 

--

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
|

Re: Grib2 Question

Roarke Gaskill

Wouldn't the the max size be limited by the number of bytes read?  So in this case 4 bytes.


Looking at netcdf's implementation they treat the value as a 32 bit signed int.


I am dealing with proprietary grib2 files that do break the current limit of 33554432.



On Tue, Nov 7, 2017 at 4:03 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 13:51:30 CET Kurt Schwehr wrote:

> It's possible to cause massive allocations with a tiny corrupted grib file

> causing an out-of-memory. I found that case with the llvm ASAN fuzzer. If

> you have a specification that gives a more reasoned maximum or a better

> overall check, I'm listening. I definitely think the sanity checking can

> be improved. Mostly I just try to survive the g2clib code. It doesn't

> come with tests and digging through GRIB specs to match up to g2clib source

> isn't my favorite thing to do.

>

> https://github.com/OSGeo/gdal/commit/ae92f7fb8e32381124a37588d27b9af695afce2

> 0

 

I guess that if Roarke is asking the question he might have a dataset that breaks this limit ? If so, we might consider reverting that change, or making it more robust (which can be very tricky I know. Perhaps some heuristics with the file size ?), or just using it in fuzzing mode and not in production for now. And a pointer to such a dataset would be much appreciated.

 

(By the way: 2<<24 is IMHO an usual way of writing a limit. I confused it with 2^24 initially. So 1 << 25 would perhaps be better. Or just in decimal form as it is completely arbitary and not related to a binary encoding)

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer


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

Re: Grib2 Question

Kurt Schwehr-2
Yeah, 1 << ## would have been better.  Sigh.

But really, someone needs to work through the logic of this stuff and do something that actually works through what is reasonable.  Code that is intelligent and explains why it is doing is far preferable.  OOM is not okay in the world I work in, so I tried to pick a large number (basically at random) that should let almost all cases through until someone took more time.

allowing a full range number without checking to see if the size is reasonable is a *major* problem for me.  An OOM is a fatal (as in no error message, your process is dead) situation in my world, so allowing larger without checking seems unpleasant.

Perhaps a config option to allow massive files through in the short term?

On Tue, Nov 7, 2017 at 2:15 PM, Roarke Gaskill <[hidden email]> wrote:

Wouldn't the the max size be limited by the number of bytes read?  So in this case 4 bytes.


Looking at netcdf's implementation they treat the value as a 32 bit signed int.


I am dealing with proprietary grib2 files that do break the current limit of 33554432.



On Tue, Nov 7, 2017 at 4:03 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 13:51:30 CET Kurt Schwehr wrote:

> It's possible to cause massive allocations with a tiny corrupted grib file

> causing an out-of-memory. I found that case with the llvm ASAN fuzzer. If

> you have a specification that gives a more reasoned maximum or a better

> overall check, I'm listening. I definitely think the sanity checking can

> be improved. Mostly I just try to survive the g2clib code. It doesn't

> come with tests and digging through GRIB specs to match up to g2clib source

> isn't my favorite thing to do.

>

> https://github.com/OSGeo/gdal/commit/ae92f7fb8e32381124a37588d27b9af695afce2

> 0

 

I guess that if Roarke is asking the question he might have a dataset that breaks this limit ? If so, we might consider reverting that change, or making it more robust (which can be very tricky I know. Perhaps some heuristics with the file size ?), or just using it in fuzzing mode and not in production for now. And a pointer to such a dataset would be much appreciated.

 

(By the way: 2<<24 is IMHO an usual way of writing a limit. I confused it with 2^24 initially. So 1 << 25 would perhaps be better. Or just in decimal form as it is completely arbitary and not related to a binary encoding)

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer




--

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

Re: Grib2 Question

Even Rouault-2

On mardi 7 novembre 2017 14:21:27 CET Kurt Schwehr wrote:

> Yeah, 1 << ## would have been better. Sigh.

>

> But really, someone needs to work through the logic of this stuff and do

> something that actually works through what is reasonable. Code that is

> intelligent and explains why it is doing is far preferable. OOM is not

> okay in the world I work in, so I tried to pick a large number (basically

> at random) that should let almost all cases through until someone took more

> time.

 

I'm afraid this is unlikely that "someone" pops up by magic to address that, given the time investment it would represent. For work related to adding a write side to the GRIB2 driver, I might try in the coming weeks to resync with newest version of g2clib & degrib, which by itself will be certainly a challenging exercice given all the patching we have done through the years, but adressing such issues of being robust with "hostile" files is definitely not in the scope of work.

 

I'm not sure if there are other C/C++ grib1+grib2 libs that would be better in that aspect, while having at least the same level of functionality, and a permissive license.

 

>

> allowing a full range number without checking to see if the size is

> reasonable is a *major* problem for me. An OOM is a fatal (as in no error

> message, your process is dead) situation in my world, so allowing larger

> without checking seems unpleasant.

>

> Perhaps a config option to allow massive files through in the short term?

 

I think the vast majority of users are likely less concerned with potential OOM situations on hostile/corrupted files, but more willing to have a working behaviour with real world datasets, so the reverse logic would seem better to me.

 

Or can we increase this threshold so that it works with datasets likes Roarke's one ? Kurt, what would be the maximum value you could bear ?

Roarke, can you add some breakpoint / debug message to see what value of *ndpts you get for your dataset ?

 

If we go with safe mode by default and a config option to disable it, then it should output a warning that explains which config option to enable to disable the limitation. + in the doc page of the driver.

 

Even

 

--

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
|

Re: Grib2 Question

Kurt Schwehr-2
Why not something like this and just let me pick a small number?

#ifdef GRIB_MAX_ALLOC
const int knMaxAllloc = GRIB_MAX_ALLOC;
#else
const int knMaxAlloc = some massive number; 
#endif

On Tue, Nov 7, 2017 at 3:18 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 14:21:27 CET Kurt Schwehr wrote:

> Yeah, 1 << ## would have been better. Sigh.

>

> But really, someone needs to work through the logic of this stuff and do

> something that actually works through what is reasonable. Code that is

> intelligent and explains why it is doing is far preferable. OOM is not

> okay in the world I work in, so I tried to pick a large number (basically

> at random) that should let almost all cases through until someone took more

> time.

 

I'm afraid this is unlikely that "someone" pops up by magic to address that, given the time investment it would represent. For work related to adding a write side to the GRIB2 driver, I might try in the coming weeks to resync with newest version of g2clib & degrib, which by itself will be certainly a challenging exercice given all the patching we have done through the years, but adressing such issues of being robust with "hostile" files is definitely not in the scope of work.

 

I'm not sure if there are other C/C++ grib1+grib2 libs that would be better in that aspect, while having at least the same level of functionality, and a permissive license.

 

>

> allowing a full range number without checking to see if the size is

> reasonable is a *major* problem for me. An OOM is a fatal (as in no error

> message, your process is dead) situation in my world, so allowing larger

> without checking seems unpleasant.

>

> Perhaps a config option to allow massive files through in the short term?

 

I think the vast majority of users are likely less concerned with potential OOM situations on hostile/corrupted files, but more willing to have a working behaviour with real world datasets, so the reverse logic would seem better to me.

 

Or can we increase this threshold so that it works with datasets likes Roarke's one ? Kurt, what would be the maximum value you could bear ?

Roarke, can you add some breakpoint / debug message to see what value of *ndpts you get for your dataset ?

 

If we go with safe mode by default and a config option to disable it, then it should output a warning that explains which config option to enable to disable the limitation. + in the doc page of the driver.

 

Even

 

--

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
|

Re: Grib2 Question

Even Rouault-2

On mardi 7 novembre 2017 15:34:15 CET Kurt Schwehr wrote:

> Why not something like this and just let me pick a small number?

>

> #ifdef GRIB_MAX_ALLOC

> const int knMaxAllloc = GRIB_MAX_ALLOC;

> #else

> const int knMaxAlloc = some massive number;

> #endif

>

 

Yes, looks reasonable. I guess "some massive number" should probably be INT_MAX if we don't have a better way to do sanity checking right now.

 

--

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
|

Re: Grib2 Question

Roarke Gaskill
It seems inappropriate (even as a quick hack) to put the size check in the grib parser. With the check there, you are not able to run simple utilities like gdalinfo on large files. What if I wanted to use gdalinfo to find out if the file is too big to process?

If it is decided to continue with this quick hack, better logging would be very helpful.

The number of points in the file I am testing is 67108864

On Tue, Nov 7, 2017 at 5:43 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 15:34:15 CET Kurt Schwehr wrote:

> Why not something like this and just let me pick a small number?

>

> #ifdef GRIB_MAX_ALLOC

> const int knMaxAllloc = GRIB_MAX_ALLOC;

> #else

> const int knMaxAlloc = some massive number;

> #endif

>

 

Yes, looks reasonable. I guess "some massive number" should probably be INT_MAX if we don't have a better way to do sanity checking right now.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer


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

Re: Grib2 Question

Even Rouault-2

On mardi 7 novembre 2017 17:45:57 CET Roarke Gaskill wrote:

> It seems inappropriate (even as a quick hack) to put the size check in the

> grib parser.

 

If we default to knMaxAllloc = INT_MAX, given that *ndpts is g2int, then the check will be a no-op. Actually the compiler and/or static analyzers will probably complain about that... So INT_MAX - 1 is probably a better choice :-)

 

 

> With the check there, you are not able to run simple utilities

> like gdalinfo on large files. What if I wanted to use gdalinfo to find out

> if the file is too big to process?

 

Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

 

--

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
|

Re: Grib2 Question

Roarke Gaskill
Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

Oh, so the OOM would happen even when calling gdalinfo? I was assuming it was later processing that was getting the OOM.

On Tue, Nov 7, 2017 at 5:58 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 17:45:57 CET Roarke Gaskill wrote:

> It seems inappropriate (even as a quick hack) to put the size check in the

> grib parser.

 

If we default to knMaxAllloc = INT_MAX, given that *ndpts is g2int, then the check will be a no-op. Actually the compiler and/or static analyzers will probably complain about that... So INT_MAX - 1 is probably a better choice :-)

 

 

> With the check there, you are not able to run simple utilities

> like gdalinfo on large files. What if I wanted to use gdalinfo to find out

> if the file is too big to process?

 

Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer


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

Re: Grib2 Question

Kurt Schwehr-2
I'm pretty sick today, so I'll hold off making the change until tomorrow (unless anyone wants to beat me to it)

-kurt

On Tue, Nov 7, 2017 at 4:02 PM, Roarke Gaskill <[hidden email]> wrote:
Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

Oh, so the OOM would happen even when calling gdalinfo? I was assuming it was later processing that was getting the OOM.

On Tue, Nov 7, 2017 at 5:58 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 17:45:57 CET Roarke Gaskill wrote:

> It seems inappropriate (even as a quick hack) to put the size check in the

> grib parser.

 

If we default to knMaxAllloc = INT_MAX, given that *ndpts is g2int, then the check will be a no-op. Actually the compiler and/or static analyzers will probably complain about that... So INT_MAX - 1 is probably a better choice :-)

 

 

> With the check there, you are not able to run simple utilities

> like gdalinfo on large files. What if I wanted to use gdalinfo to find out

> if the file is too big to process?

 

Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer




--

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

Re: Grib2 Question

Kurt Schwehr-2

On Tue, Nov 7, 2017 at 8:03 PM, Kurt Schwehr <[hidden email]> wrote:
I'm pretty sick today, so I'll hold off making the change until tomorrow (unless anyone wants to beat me to it)

-kurt

On Tue, Nov 7, 2017 at 4:02 PM, Roarke Gaskill <[hidden email]> wrote:
Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

Oh, so the OOM would happen even when calling gdalinfo? I was assuming it was later processing that was getting the OOM.

On Tue, Nov 7, 2017 at 5:58 PM, Even Rouault <[hidden email]> wrote:

On mardi 7 novembre 2017 17:45:57 CET Roarke Gaskill wrote:

> It seems inappropriate (even as a quick hack) to put the size check in the

> grib parser.

 

If we default to knMaxAllloc = INT_MAX, given that *ndpts is g2int, then the check will be a no-op. Actually the compiler and/or static analyzers will probably complain about that... So INT_MAX - 1 is probably a better choice :-)

 

 

> With the check there, you are not able to run simple utilities

> like gdalinfo on large files. What if I wanted to use gdalinfo to find out

> if the file is too big to process?

 

Unfortunately given how the GRIB degrib and underlying libaries used are done, you cannot get metadata without processing the whole file.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--
Roarke Gaskill  |Senior Software Engineer




--



--

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