General impressions
~~~~~~~~~~~~~~~~~~~

  Many (all ?) of the symbols exposed in library functions are systemd
  shims and unused within the codebase.

  Would be nice to have a configure switch to enable/disable building
  of the 'systemd shim' portion of LoginKit.

  LoginKit should probably bring it's own philosophy about how it's
  really intended to be used, provide some library API where it's needed
  and have the systemd APIs as optional at build/install time.

  __attribute__((visibility ("default"))) is used in many places, what
  is the reason for this ?

  It seems just convoluted, globally defined symbols are visible by
  default in any library, why the extra compiler hint ?


common
~~~~~~

  common/bus.c
  ~~~~~~~~~~~~

  This seems to get the GDBusConnection for the system bus which generally stays alive
  for the calling program's lifetime, in places you have a library destructor which
  explicitly closes the bus.

  I would suggest using the GOnce pattern here, so that the bus is simply accquired the first
  time to resolve the bus and perhaps move that library destructor code to the same place.


loginkit-daemon
~~~~~~~~~~~~~~~

Not much to say about this directory, mostly systemd shim stuff.


loginkit-login
~~~~~~~~~~~~~~

  Watch the memory in this directory.
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  pid.c: get_session_by_pid() returns an object path string allocated by glib
         which needs to be freed with g_free()

         sd_pid_get_session() does: *session = strdup (*session) and leaks
         the string returned by get_session_by_pid().

         sd_pid_get_owner_uid() and sd_pid_get_machine_name() also
         leaks the session path string.

  session.c: get_seat() Similar issues as get_session_by_pid(), simply leaking
             glib allocated data with *seat = strdup (*seat).


  For the above issues, suggest using the special '&' syntax for getting the
  string from the variant without any memory allocation, before unreffing
  the variant itself.

  For example, in get_seat() you could have:

        /* Get a pointer to the memory holding the string in the variant */
        g_variant_get (reply, "(&o)", seat);

        /* Allocate a copy on glibc's memory heap */
        *seat = strdup (*seat);

        /* Now safely free the variant */
        g_variant_unref (reply);


  seat.c: sd_seat_get_sessions()

       There are multiple memory management issues here:

       a.) Fetch the count at the beggining as you do to determine the size
           of the returned vectors/arrays.

       b.) Use the while loop as documented in g_variant_iter_loop(), otherwise
           you will invariably leak the last value as g_variant_iter_loop() does
           not have a chance to free it for you when hitting the last item.

       c.) You are assuming that the string pointer handed to you in g_variant_iter_loop()
           is actually given to you, but it's not - as g_variant_iter_loop() documentation
           specifies that you need not free the values passed between iterations.

           This is actually practical for you, as you will want a copy on glibc's heap

       d.) In the case of an error encountered in the loop, you free the 'sessions'
           vector but fail to free the allocated strings in that vector.

       e.) You need to ensure the memory returned from this function is on the correct
           heap, right now you are returning memory allocated by glib, which will, I assume,
           later be freed with glibc functions like 'free()'.

           Note that:

              char *ptr = g_strdup ("pony");
              free (ptr);

           Is incorrect and will cause memory corruption and faults.

           You need to allocate those vectors before the loop with glibc functions
           and ensure that the strings you assign are copied with strdup() as you
           already do elsewhere.


loginkit
~~~~~~~~

More memory related issues around here...

  power.c: handle_can() returns allocated string which is never freed.

I didnt look through all the various files here, possibly there are other
memory management related issues.

  loginkitd.c: I dont think you want or need to be using the convinence
               bus_get() API here.

               Instead, you actually *have* the bus when on_name_acquired()
               and on_name_lost() is called - use that bus to export and
               unexport your interface skeleton and to subscribe and
               unsubscribe from signals.

               I'm also unsure if you want to quit the mainloop based on
               losing the bus name.

               Instead I would probably use g_unix_signal_add_full()
               to subscribe to the SIGTERM signal and then explicitly
               unown the bus name, causing everything to come down,
               and then eventually quit.