brandon-rhodes / python-sgp4

Python version of the SGP4 satellite position library
MIT License
376 stars 88 forks source link

Invalid use of strcpy in the C++ code #132

Closed bluescarni closed 3 days ago

bluescarni commented 1 week ago

Hello!

Today I compiled the sgp4 C++ code from source with the address sanitizer enabled, and the following error was reported:

==3225==ERROR: AddressSanitizer: strcpy-param-overlap: memory ranges [0x619002fc33a0,0x619002fc33a6) and [0x619002fc33a0, 0x619002fc33a6) overlap
    #0 0x7f29d9c82054 in __interceptor_strcpy ../../../../libsanitizer/asan/asan_interceptors.cpp:438
    #1 0x7f298dd914ac in SGP4Funcs::sgp4init(gravconsttype, char, char const*, double, double, double, double, double, double, double, double, double, double, elsetrec&) extension/SGP4.cpp:1437
    #2 0x7f298dd99ec5 in SGP4Funcs::twoline2rv(char*, char*, char, char, char, gravconsttype, double&, double&, double&, elsetrec&) extension/SGP4.cpp:2437
    #3 0x7f298dd9bcab in Satrec_twoline2rv extension/wrapper.cpp:152
    #4 0x563d682c60c8 in cfunction_call /usr/local/src/conda/python-3.12.7/Objects/methodobject.c:548
[...]

The sanitizer is complaining that strcpy() is being used with identical source and destination arguments, which is something that one is not supposed to do:

https://www.man7.org/linux/man-pages/man3/strcpy.3.html

(see the CAVEATS section at the bottom)

This is the offending line (1437 in SGP4.cpp):

strcpy(satrec.satnum, satn);

Which, in turn, is part of the invocation of sgp4init() at line 2437 in SGP4.cpp:

        // ---------------- initialize the orbit at sgp4epoch -------------------
        sgp4init(whichconst, opsmode, satrec.satnum, (satrec.jdsatepoch + satrec.jdsatepochF) - 2433281.5, satrec.bstar,
            satrec.ndot, satrec.nddot, satrec.ecco, satrec.argpo, satrec.inclo, satrec.mo, satrec.no_kozai,
            satrec.nodeo, satrec);

So, satrec.satnum ultimately ends up being copied onto itself, hence the sanitizer error.

The fix should be as simple as skipping the strcpy() altogether if the source and destination pointers are the same. Is this the correct place so submit a PR or should I bring the issue directly to Vallado/celestrak?

brandon-rhodes commented 3 days ago

Is this the correct place so submit a PR or should I bring the issue directly to Vallado/celestrak?

Alas, this python-sgp4 project merely packages the upstream C++ code for consumption by Python programmers. It doesn't make any changes to the code, lest its behavior from Python differ in any way from its behavior when called from other languages. You will want to offer a fix upstream to Vallado. I'll be happy to update python-sgp4 with new upstream code whenever that is released.

brandon-rhodes commented 3 days ago

(And, thanks for the well-written description! I wouldn't have known quite what to do had I seen only the error message.)