Opened 2 years ago

Closed 2 years ago

#34026 closed Bug (fixed)

WKBReader.read() crashes on strings.

Reported by: Benoît Vinot Owned by: Leo Tom
Component: GIS Version: 4.1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Benoît Vinot)

Hi,

There is a potential type error in the implementation of the read method of the WKBReader class in contrib.gis. Here is a small example to create the problem:

from django.contrib.gis.geos.prototypes.io import _WKBReader
wkb='0101000020E6100000C07DD40B0FA8F5BF8794C5DD53DD4740' # <-- string
_WKBReader().read(wkb)

This code works fine:

from django.contrib.gis.geos.prototypes.io import _WKBReader
wkb='0101000020E6100000C07DD40B0FA8F5BF8794C5DD53DD4740'
_WKBReader().read(wkb.encode()) # <--- Converted into bytes

I suggest replacing the current implementation of this method by:

    def read(self, wkb):
        "Return a _pointer_ to C GEOS Geometry object from the given WKB."
        if isinstance(wkb, memoryview):
            wkb_s = bytes(wkb)
            return wkb_reader_read(self.ptr, wkb_s, len(wkb_s))
        elif isinstance(wkb, bytes):
            return wkb_reader_read_hex(self.ptr, wkb, len(wkb))
        elif isinstance(wkb, str):
            wkb_s = wkb.encode() # <------------------- Ensure bytes
            return wkb_reader_read_hex(self.ptr, wkb_s, len(wkb_s))
        else:
            raise TypeError

Change History (15)

comment:1 by Benoît Vinot, 2 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 2 years ago

Triage Stage: UnreviewedAccepted

Agreed, it doesn't work as documented.

comment:3 by Mariusz Felisiak, 2 years ago

Summary: WKBReader Type ErrorWKBReader.read() crashes on strings.

comment:4 by Benoît Vinot, 2 years ago

Thanks. Do you want me to create a PR?

in reply to:  4 ; comment:5 by Leo Tom, 2 years ago

Replying to Benoît Vinot:

Thanks. Do you want me to create a PR?

Can I claim and work on this issue ?

Version 0, edited 2 years ago by Leo Tom (next)

in reply to:  4 comment:6 by Mariusz Felisiak, 2 years ago

Replying to Benoît Vinot:

Thanks. Do you want me to create a PR?

Please do, thanks!

in reply to:  5 comment:7 by Benoît Vinot, 2 years ago

Replying to Leo Tom:

Replying to Benoît Vinot:

Thanks. Do you want me to create a PR?

Can I claim and work on this issue ?

Yes, if you want. :-)

comment:8 by Leo Tom, 2 years ago

Owner: changed from nobody to Leo Tom
Status: newassigned

comment:9 by Leo Tom, 2 years ago

Has patch: set

comment:11 by Tim Graham, 2 years ago

Needs tests: set

Please uncheck "Needs tests" when you add one to the PR.

in reply to:  11 comment:12 by Leo Tom, 2 years ago

Replying to Tim Graham:

Please uncheck "Needs tests" when you add one to the PR.

I just have to write a test in django/tests/gis_tests/geos_tests/test_io.py right. So all I have to do is inside the test03_wkbreader function add a string as well right?

comment:13 by Leo Tom, 2 years ago

Needs tests: unset

comment:14 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In f3822d4a:

Fixed #34026 -- Fixed WKBReader.read() crash on string input.

Note: See TracTickets for help on using tickets.
Back to Top