Fix segfault accessing null pointer in IceGrid node updateRevision

[pepone]

Fix #4547

[Copilot]

Pull Request Overview

This PR addresses a potential null pointer dereference in the updateRevision method by adding a null check for _desc before accessing it. The change recognizes that _desc can be nullptr if the server's initial load has not completed yet.

Key changes:

• Added null check guard for _desc before updating uuid and revision
• Introduced local variable desc to avoid repeated member access
• Applied the same pattern to the _load path for consistency

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[externl]

I'm not quite following. Can't desc be null here still?

[pepone]

This is just inside updateRevision writing the revision file, if it was null before, it will still be null. This method doesn't update _desc.

[externl]

But it _load is false, then desc will be nullptr. This deference will crash.

[pepone]

_desc is set if already loaded.
_load is set if loading.

We don't call this method if neither is set, I will add an assert.

[pepone]

see:

    auto d = _load ? _load->getInternalServerDescriptor() : _desc;
    if (d && (replicaName != "Master" || d->sessionId == desc->sessionId) && !descriptorUpdated(d, desc))
    {
        if (d->revision != desc->revision)
        {
            updateRevision(desc->uuid, desc->revision);
        }

The if (d && ensures either _load or _desc is set.

Next call is:

_load->addCallback(response, exception); // Must be called before startRuntimePropertiesUpdate!
updateRevision(desc->uuid, desc->revision);

Here _load is set.

The third call hasppens inside updateImpl and both are set in this case:

void
ServerI::updateImpl(const shared_ptr<InternalServerDescriptor>& descriptor)
{
    assert(_load && descriptor);

    _desc = descriptor;
    _waitForReplication = true;

    // Remember if the server was just released by a session, this will be used later to not update the configuration
    // on the disk (as an optimization and to allow users to review the configuration file after allocating a server --
    // that's useful if the server configuration is bogus and the session server can't start).
    bool serverSessionReleased = _desc && _desc->activation == "session" && _desc->revision == descriptor->revision &&
                                 !_desc->sessionId.empty() && descriptor->sessionId.empty();

[bernardnormier]

I find this helper desc (a shared_ptr) pretty confusing. You're still updating _desc when it's not null.
And when _load is set, you are updating load's internal server descriptor (not the greatest API).

What about avoiding this desc helper and writing instead something like:

string application;

if (_desc)
{
    _desc->uuid = uuid;
    _desc->revision = revision;
    application = _desc->application;
}

if (_load)
{
    _load->getInternalServerDescriptor()->uuid = uuid;
    _load->getInternalServerDescriptor()->revision = revision;
  
    // When both _desc and _load are not null, this one prevails.
    application = _load->getInternalServerDescriptor()->application;
}

and then use application, uuid and revision in the code below?