Initialize `prop_id_` in `NPyMechObj`. #2604

1uc · 2023-11-08T10:38:11Z

This PR fixes initialization of NPyMechObj. The Python API seems to perform the equivalent of a malloc followed by calling a user supplied init(NPyMechObj *). In the init function:

nrn/src/nrnpython/nrnpy_nrn.cpp

Lines 264 to 275 in fa132e4

    
           static NPyMechObj* new_pymechobj(NPySegObj* pyseg, Prop* p) { 
        
               NPyMechObj* m = PyObject_New(NPyMechObj, pmech_generic_type); 
        
               if (!m) { 
        
                   return NULL; 
        
               } 
        
               m->pyseg_ = pyseg; 
        
               Py_INCREF(m->pyseg_); 
        
               m->prop_ = p; 
        
               m->prop_id_ = p->id(); 
        
               m->type_ = p->_type; 
        
               return m; 
        
           }

assigns a to prop_id_, which at that point I believe is filled with random values. This can cause either a segfault, or freeing some random address.

The solution pursued here is to use placement new:
https://en.cppreference.com/w/cpp/language/new
(There's a section "Placement New" halfway down the document.)

and if needed it'll call the destructor manually.

1uc · 2023-11-08T10:39:43Z

This locally fixes the segfault in one test that reliably used to cause a segfault. The next steps are to make sure that this doesn't leak, because we might need to call the dtor manually. Then if there's more segfault, I'll deal with those.

codecov · 2023-11-08T11:45:25Z

Codecov Report

Merging #2604 (d409241) into master (9700e91) will increase coverage by 0.00%.
The diff coverage is 81.81%.

@@           Coverage Diff           @@
##           master    #2604   +/-   ##
=======================================
  Coverage   66.19%   66.19%           
=======================================
  Files         559      559           
  Lines      108933   108939    +6     
=======================================
+ Hits        72105    72117   +12     
+ Misses      36828    36822    -6

Files	Coverage Δ
src/nrnpython/nrnpy_nrn.cpp	`74.65% <81.81%> (+0.04%)`	⬆️

... and 6 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

src/nrnpython/nrnpy_nrn.cpp

When creating a `NPyMechObj` the Python C API will perform the equivalent of a `malloc(sizeof(NPyMechObj))`. This will return an invalid object, that must be fixed using placement new. Details: * `PyObject_New(NPyMechObj, pmech_generic_type)` does not call `tp_new`. * `tp_free` is the mental equivalent of `free`. * Creating a valid NPyMechObj object should be done via `new_mechobj()`.

Since `NPyMechObj` contains a `ob_base` I'm not convinced it's correct to default initialize the entire struct.

nrnhines

I believe you fixed it !

src/nrnpython/nrnpy_nrn.cpp

1uc · 2023-11-09T11:14:34Z

There is one question I didn't answer:
#2604 (comment)

Yes, I think we should avoid the warning. The way I get around this is by converting:

typedef struct {
  ...
} NPyMechObj;

to

struct NPyMechObj {
  ...
};

and removing the {} initializers from the other structs.

What made me unsure about the first change is the mention of "external linkage" and "for linkage purposes" on cppreference. To my knowledge structs aren't linked. However, in C++ their name is used to do name-mangling, which affects linkage and I think that's what they're referring to. Therefore, I think the change is fine. The reason why the Python C API uses the pattern is because it allows us to say NPyMechObj instead of struct NPyMechObj and additionally, we can't accidentally name a function NPyMechObj. My conclusion is that the one is good C and the other good C++; but their byte-wise layout is identical.

Justification for removing the {}: I can't find place where we do NPyMechObj obj; or similar, I don't think we should do that because, I suspect we need to allow the Python C API to initialize the ob_base. Hence, the the argument is that {} does nothing (and might be misleading in the sense that it looks like it has sensible default initialization when it doesn't/can't).

If something doesn't sound right, please let me know so we can dig deeper.

1uc · 2023-11-09T11:15:05Z

I'll run the NMODL CI with Python 3.12 on this PR.

azure-pipelines · 2023-11-09T11:32:04Z

✔️ 67b39de -> Azure artifacts URL

nrnhines · 2023-11-09T12:54:07Z

If something doesn't sound right, please let me know so we can dig deeper.

Your explanation seems reasonable to me. I believe it also may be useful to have code comments about why the pattern (that we use everywhere else) changed from typedef struct {...} NPtMechObj to struct NPyMechObj{...}.

I experimented a bit more by removing the initializer in the struct from prop_id_{}; to prop_id_;. It should not (and did not) matter since we are initializing explicitly in the constructor wrapper static NPyMechObj* new_pymechobj() {.
Furthermore, without the initializer in the typedef struct form, there is no warning about c linkage which kind of semi-surprised me. But I guess is consistent with a closer reading of typedef-name for linkage purposes that I copy/pasted in a previous comment.
And I guess a comment in the constructor wrapper about placement new and corresponding explicit destructor call in NPyMechObj_dealloc might be in order as well :) if I can compose something that would fit on a single line.

I see that this file is still -Wall warning free (except for unused variable and function). By the way, ninja is very useful in this regard since it is easy to edit the build_ninja file, search for a file and modify FLAGS for that specific file compile build rule.

In summary, I tentatively propose: (if substantively acceptable, feel free to improve the comments. If you think it is more or less pointless, I accept your current version)

typedef struct {           
    PyObject_HEAD 
    NPySegObj* pyseg_;
    Prop* prop_;
    // Following cannot be initialized when NPyMechObj allocated by Python. See new_pymechobj wrapper.
    neuron::container::non_owning_identifier_without_container prop_id_;  
    int type_;
} NPyMechObj;

static void NPyMechObj_dealloc(NPyMechObj* self) {
    Py_XDECREF(self->pyseg_);
    // must manually call destructor since it was manually constructed in new_pymechobj wrapper
    self->prop_id_.~non_owning_identifier_without_container();
    ((PyObject*) self)->ob_type->tp_free((PyObject*) self);
}

static NPyMechObj* new_pymechobj() {
    NPyMechObj* m = PyObject_New(NPyMechObj, pmech_generic_type);
    if (m) {
        //  "placement new" idiom since Python C allocation cannot call the initializer to start it as "null".
        // So later a = b might segfault because copy constructor decrements the refcount of a's nonsense memory.
        new (&m->prop_id_) neuron::container::non_owning_identifier_without_container;
    }
    return m;
}

1uc · 2023-11-09T13:18:08Z

I like the comments.

~~I'm not (yet) convinced by the absence of the warning.~~ As you write, precise reading is required.

1uc · 2023-11-09T13:30:45Z

The previous failing CI has is running against this version; and some tests that used to segfault now pass. (Got cancelled by pushing commits to this repository.)

azure-pipelines · 2023-11-09T14:23:12Z

✔️ b5cce09 -> Azure artifacts URL

1uc · 2023-11-09T14:32:48Z

The NMODL CI is green:
BlueBrain/nmodl#1096

sonarqubecloud · 2023-11-11T18:51:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

azure-pipelines · 2023-11-11T19:27:59Z