fix two buffer overflow risks

johannct commented 2 months ago

Closes #189

Change Description

[x] My PR includes a link to the issue that I am addressing

Solution Description

Code Quality

[ ] I have read the Contribution Guide
[ ] My code follows the code style of this project
[ ] My code builds (or compiles) cleanly without any errors or warnings
[ ] My code contains relevant comments and necessary documentation

Project-Specific Pull Request Checklists

Bug Fix Checklist

[ ] My fix includes a new test that breaks as a result of the bug (if possible)
[ ] My change includes a breaking change
- [ ] My change includes backwards compatibility and deprecation warnings (if possible)

New Feature Checklist

[ ] I have added or updated the docstrings associated with my feature using the NumPy docstring format
[ ] I have updated the tutorial to highlight my new feature (if appropriate)
[ ] I have added unit/End-to-End (E2E) test cases to cover my new feature
[ ] My change includes a breaking change
- [ ] My change includes backwards compatibility and deprecation warnings (if possible)

Documentation Change Checklist

[ ] Any updated docstrings use the NumPy docstring format

Build/CI Change Checklist

[ ] If required or optional dependencies have changed (including version numbers), I have updated the README to reflect this
[ ] If this is a new CI setup, I have added the associated badge to the README

Other Change Checklist

[ ] Any new or updated docstrings use the NumPy docstring format.
[ ] I have updated the tutorial to highlight my new feature (if appropriate)
[ ] I have added unit/End-to-End (E2E) test cases to cover any changes
[ ] My change includes a breaking change
- [ ] My change includes backwards compatibility and deprecation warnings (if possible)

johannct commented 2 months ago

This needs proper unit testing

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.65%. Comparing base (5142d1e) to head (a2988e0). Report is 27 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #192 +/- ## ========================================== + Coverage 66.29% 66.65% +0.35% ========================================== Files 50 50 Lines 6169 6229 +60 Branches 928 937 +9 ========================================== + Hits 4090 4152 +62 + Misses 2079 2077 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

johannct commented 1 month ago

Ok @hdante the credible_interval code has been simplified, and the problematic lines are gone : the PDF always has a uniform x grid, so we can speed up and simplify the search of the val index. Any chance you can fire you test?

hdante commented 1 month ago

Johann, yes, but due to other tasks only on Friday or next week.

johannct commented 1 month ago

no hurry

hdante commented 1 month ago

Hello, Johann, I've recompiled the lephare library with the address sanitizer and reexecuted the original test, but I got a buffer overflow in PDF::creadible_interval(). I haven't read the code so I don't know yet what's happening.

==33078==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d002b6ebe8 at pc 0x2b5828cbc6c3 bp 0x7ffc6ec04ec0 sp 0x7ffc6ec04eb8
READ of size 8 at 0x61d002b6ebe8 thread T0
    #0 0x2b5828cbc6c2 in std::_Vector_base<double, std::allocator<double> >::~_Vector_base() /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/stl_vector.h:336
    #1 0x2b5828cbc6c2 in std::vector<double, std::allocator<double> >::~vector() /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/stl_vector.h:683
    #2 0x2b5828cbc6c2 in PDF::credible_interval(float, double) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/PDF.cpp:391
    #3 0x2b5828c732a5 in onesource::mode() /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/onesource.cpp:1138
    #4 0x2b5828d38eac in PhotoZ::run_photoz(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/photoz_lib.cpp:1620
    #5 0x2b5828fd2410 in pybind11::cpp_function::cpp_function<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}::operator()(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&) const /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/pybind11.h:154
    #6 0x2b5828fd2410 in void pybind11::detail::argument_loader<PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&>::call_impl<void, pybind11::cpp_function::cpp_function<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&, 0ul, 1ul, 2ul, 3ul, pybind11::detail::void_type>(pybind11::cpp_function::cpp_function<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&, std::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>, pybind11::detail::void_type&&) && /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/cast.h:1506
    #7 0x2b5828fd2410 in std::enable_if<std::is_void<void>::value, pybind11::detail::void_type>::type pybind11::detail::argument_loader<PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&>::call<void, pybind11::detail::void_type, pybind11::cpp_function::cpp_function<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&>(pybind11::cpp_function::cpp_function<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&) && /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/cast.h:1480
    #8 0x2b5828fd2410 in pybind11::cpp_function::initialize<pybind11::cpp_function::initialize<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}, void, PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(pybind11::cpp_function::initialize<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&&, void (*)(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(pybind11::detail::function_call&)#3}::operator()(pybind11::detail::function_call&) const /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/pybind11.h:297
    #9 0x2b5828fd2410 in pybind11::cpp_function::initialize<pybind11::cpp_function::initialize<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}, void, PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(pybind11::cpp_function::initialize<void, PhotoZ, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&, pybind11::name, pybind11::is_method, pybind11::sibling>(void (PhotoZ::*)(std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&)#1}&&, void (*)(PhotoZ*, std::vector<onesource*, std::allocator<onesource*> >, std::vector<double, std::allocator<double> > const&, std::vector<double, std::allocator<double> > const&), pybind11::name const&, pybind11::is_method const&, pybind11::sibling const&)::{lambda(pybind11::detail::function_call&)#3}::_FUN(pybind11::detail::function_call&) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/pybind11.h:267
    #10 0x2b5828ead816 in pybind11::cpp_function::dispatcher(_object*, _object*, _object*) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/extern/pybind11/include/pybind11/pybind11.h:989
    #11 0x528766 in cfunction_call /usr/local/src/conda/python-3.11.9/Objects/methodobject.c:542
    #12 0x5041ab in _PyObject_MakeTpCall /usr/local/src/conda/python-3.11.9/Objects/call.c:214
    #13 0x5116e6 in _PyEval_EvalFrameDefault /usr/local/src/conda/python-3.11.9/Python/ceval.c:4769
    #14 0x5cbed9 in _PyEval_EvalFrame /usr/local/src/conda/python-3.11.9/Include/internal/pycore_ceval.h:73
    #15 0x5cbed9 in _PyEval_Vector /usr/local/src/conda/python-3.11.9/Python/ceval.c:6434
    #16 0x5cb5ae in PyEval_EvalCode /usr/local/src/conda/python-3.11.9/Python/ceval.c:1148
    #17 0x5ec6a6 in run_eval_code_obj /usr/local/src/conda/python-3.11.9/Python/pythonrun.c:1741
    #18 0x5e823f in run_mod /usr/local/src/conda/python-3.11.9/Python/pythonrun.c:1762
    #19 0x5fd191 in pyrun_file /usr/local/src/conda/python-3.11.9/Python/pythonrun.c:1657
    #20 0x5fc55e in _PyRun_SimpleFileObject /usr/local/src/conda/python-3.11.9/Python/pythonrun.c:440
    #21 0x5fc282 in _PyRun_AnyFileObject /usr/local/src/conda/python-3.11.9/Python/pythonrun.c:79
    #22 0x5f6efd in pymain_run_file_obj /usr/local/src/conda/python-3.11.9/Modules/main.c:360
    #23 0x5f6efd in pymain_run_file /usr/local/src/conda/python-3.11.9/Modules/main.c:379
    #24 0x5f6efd in pymain_run_python /usr/local/src/conda/python-3.11.9/Modules/main.c:601
    #25 0x5f6efd in Py_RunMain /usr/local/src/conda/python-3.11.9/Modules/main.c:680
    #26 0x5bbc78 in Py_BytesMain /usr/local/src/conda/python-3.11.9/Modules/main.c:734
    #27 0x2b58119e3554 in __libc_start_main (/lib64/libc.so.6+0x22554)
    #28 0x5bbac2  (/lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/bin/python3.11+0x5bbac2)

0x61d002b6ebe8 is located 0 bytes to the right of 2408-byte region [0x61d002b6e280,0x61d002b6ebe8)
allocated by thread T0 here:
    #0 0x2b5810779d77 in operator new(unsigned long) /opt/conda/conda-bld/gcc-compiler_1654084175708/work/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x2b5828cc2ac0 in __gnu_cxx::new_allocator<double>::allocate(unsigned long, void const*) /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/ext/new_allocator.h:127
    #2 0x2b5828cc2ac0 in std::allocator_traits<std::allocator<double> >::allocate(std::allocator<double>&, unsigned long) /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/alloc_traits.h:460
    #3 0x2b5828cc2ac0 in std::_Vector_base<double, std::allocator<double> >::_M_allocate(unsigned long) /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/stl_vector.h:346
    #4 0x2b5828cc2ac0 in double* std::vector<double, std::allocator<double> >::_M_allocate_and_copy<__gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > > >(unsigned long, __gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >, __gnu_cxx::__normal_iterator<double const*, std::vector<double, std::allocator<double> > >) /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/stl_vector.h:1511
    #5 0x2b5828cc2ac0 in std::vector<double, std::allocator<double> >::operator=(std::vector<double, std::allocator<double> > const&) /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/vector.tcc:226

SUMMARY: AddressSanitizer: heap-buffer-overflow /lustre/t1/cl/lsst/tmp/henrique.almeida/miniconda3/x86_64-conda-linux-gnu/include/c++/11.2.0/bits/stl_vector.h:336 in std::_Vector_base<double, std::allocator<double> >::~_Vector_base()
Shadow bytes around the buggy address:
  0x0c3a80565d20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565d30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565d40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565d50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565d60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3a80565d70: 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]fa fa
  0x0c3a80565d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a80565d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565db0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a80565dc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==33078==ABORTING

hdante commented 1 month ago

Note: here is the exact commit that I've used: fe38ff6804a114ca4aa921e7f56c6cbcdd875633

johannct commented 1 month ago

Note: here is the exact commit that I've used: fe38ff6

Hmmm L.391 is the end of the function, so it looks like the sanitizer is complaining when freeing local variables memory; it complains about vector destruction, so it does not seem to be related to the returned pair variable. I am not sure I understand the code anymore, I think it is bugged. I will get back to you when I am happy with it again. In the meantime can you add the address sanitizer compilation control in CMakeLists in the same manner as I did for the thread sanitizer?

johannct commented 1 month ago

It should be correct now, I believe. If you get buffer overflow again, then it will mean that there are corner cases that I need to understand better

johannct commented 1 month ago

I decided to remove the nagging test issue with empty args passed to the parser, as this seems a can of worm : the arguments passed to pytest are captured by the code in runner.py, which bombs. On the other hand python -m pytest explicitely stops the option list. Likewise on notebook options leak into the runner parser, etc.....

hdante commented 1 month ago

Johann, I won't be able to test these patches this week because the LineA environment is offline for maintenance. I can't really review these patches without understanding the original algorithm in more detail, but I won't have enough time for that. I've noticed that from the original pull request of fixing the two buffer overflows, one was fixed in the resulting code and the other was refactored so that the original code was deleted, so I confirm that the original issue was fixed. Now, to continue, we may either wait for the LineA cluster come back online, which will happen hopefully but not certainly in the next week, or commit all the current changes and, if necessary, rerun it with the address sanitizer and continue debugging from a new issue later.

Meanwhile, I'll open a new pull request with the cmake configuration for the address sanitizer.

johannct commented 1 month ago

ok @hdante I ask @raphaelshirley to look at it, but I agree that at this stage it is probably better to merge it and take a clean main for further studies.

johannct commented 1 month ago

I have a couple of questions but I am willing to merge given all tests are passing and new test coverage has been added. Should we be using HIGH_CHI2 in place of 1.e9 everywhere e.g.:

You are absolutely right that we should be consistent in such usage. Can you open an issue to that effect please? Better to avoid adding more to this PR

lephare-photoz / lephare