bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Pierre,

Pierre Langlois <pierre.langlois@gmx.com> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

[...]

>> The change was reinstated as part of the mcron update, in
>> 44554e7133aa60e1b453436be1e80394189cabd9.  The bit that seems to cause
>> the issue here (still not clearly understood) is probably this one:
>>
>> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
>> index 636c49ccba..dacfc52ba9 100644
>> --- a/gnu/services/configuration.scm
>> +++ b/gnu/services/configuration.scm
>> @@ -242,17 +242,17 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
>>                 stem
>>                 #,(id #'stem #'make- #'stem)
>>                 #,(id #'stem #'stem #'?)
>> -               (%location #,(id #'stem #'stem #'-location)
>> -                          (default (and=> (current-source-location)
>> -                                          source-properties->location))
>> -                          (innate))
>>                 #,@(map (lambda (name getter def)
>>                           #`(#,name #,getter (default #,def)
>>                                     (sanitize
>>                                      #,(id #'stem #'validate- #'stem #'- 
>> name))))
>>                         #'(field ...)
>>                         #'(field-getter ...)
>> -                       #'(field-default ...)))
>> +                       #'(field-default ...))
>> +               (%location #,(id #'stem #'stem #'-location)
>> +                          (default (and=> (current-source-location)
>> +                                          source-properties->location))
>> +                          (innate)))
>>
>>               (define #,(id #'stem #'stem #'-fields)
>>                 (list (configuration-field
>>
>>
>> Reverting it would likely fix the issue (haven't tried), but it'd be
>> nice to have a clear understanding of what's going on.  It may have
>> unmasked a bug waiting to bite.
>>
>> The issue seems to be with the serialization of the
>> <namespace-configuration> object nested in the <dovecot-configuration>
>> record.  I tried this at the REPL:
>>
>> scheme@(guile-user)> ,m (gnu services mail)
>> scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
>> $8 = #<<namespace-configuration> name: "inbox" type: "private" separator: "" 
>> prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: #t 
>> mailboxes: () %location: #f>
>> scheme@(gnu services mail)> (serialize-configuration $8 
>> namespace-configuration-fields)
>> name=inbox
>> type=private
>> separator=
>> prefix=
>> location=#f
>
> The location here should probably be empty rather than `#f' no? It looks
> as though the value is coming from the internal %location, rather than
> the user-provided location.

Good eye!  Perhaps my earlier simple session was able to reproduce after
all!  #f sure doesn't read as a successfully serialized value :-).  It
probably came from %location, which is set to #f when working at the
REPL.

> If the two fields can shadow each other,
> then indeed, that looks like an existing bug that was exposed by the
> reordering, rather than a bug with the reorder itself.
>
> I'll if I can find anything the macro, it looks quite complex to me :-).

It's not only to you, if that helps.  It's rather... intimidating ^^'.

Looking at it again, the problem is not so mysterious after all... The
%location field has its accessor set to be:

--8<---------------cut here---------------start------------->8---
               (%location #,(id #'stem #'stem #'-location)
                          (default (and=> (current-source-location)
                                          source-properties->location))
                          (innate)))
--8<---------------cut here---------------end--------------->8---

#,(id #'stem #'stem #'-location)

which gets expanded to namespace-configuration-location, which shadows
that of the now preceding "location" field.

The bug in the previous condition would have been reversed; the source
location would have been shadowed by the "location" field value.

Ludovic, would you have an idea of where the %location field or its
CONFIGURATION-location accessor come into play?  I tried tracing it in
the source, but I only see it being set and the location being pulled
from the sanitizer via "(datum->syntax #'value (syntax-source #'value)"
in (gnu services configuration) around line 227, which is the location
that would get printed in the error handler CALL-WITH-ERROR-HANDLING
from (guix ui):

--8<---------------cut here---------------start------------->8---
             ((formatted-message? c)
              (apply report-error
                     (and (error-location? c) (error-location c))
                     (gettext (formatted-message-string c) %gettext-domain)
                     (formatted-message-arguments c))
              (when (fix-hint? c)
                (display-hint (condition-fix-hint c)))
              (exit 1))
--8<---------------cut here---------------end--------------->8---

I could be wrong, but since the "location" of a field appears to be an
"intrinsic" property rather than something explicitly attached to it,
perhaps there's no need for a "location" accessor?  Or it could be named
differently, such as "%location" to reduce the risk of clashing with
user-defined fields.

Does that make sense?

-- 
Thanks,
Maxim