bug#24656: 25.1; Emacs leaves lock files on a CIFS share

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Jan Synáček]

Emacs leaves lock files on a CIFS share:

1) Setup a simple samba share (/etc/samba/smb.conf):
...
[public]
 browsable = yes
 guest ok = yes
 path = <path to the share>
 read only = no

2) Mount it:
# mount -t cifs //<server>/public /mnt/tmp -o guest

3) # echo "hello emacs" > /mnt/tmp/test.txt

4) Fire up emacs -Q and start editing /mnt/tmp/test.txt. This is what I
see on my system:

# ls -la /mnt/tmp/
total 9
drwxrwxrwx+ 2 root      root       0 Oct 10 13:21 .
drwxr-xr-x. 1 root      root      20 Jul 15 10:33 ..
lrwxrwxrwx. 1 nfsnobody nfsnobody 54 Oct 10 13:21 .#test.txt -> jsynacek@jsynacek-ntb.brq.redhat.com.57671476081527
-rw-rw-rw-+ 1 jsynacek  users     10 Oct 10 13:16 test.txt

When I save the file and quit emacs, the lock file stays there. It
should be removed. Maybe it's because of the weird 0xF022 sequence in
the file name?

Originally reported at
https://bugzilla.redhat.com/show_bug.cgi?id=1271407.

-- 
Jan Synacek
Software Engineer, Red Hat

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> From: jsynacek@redhat.com (Jan Synáček)
> Date: Mon, 10 Oct 2016 13:30:24 +0200
> 
> # ls -la /mnt/tmp/
> total 9
> drwxrwxrwx+ 2 root      root       0 Oct 10 13:21 .
> drwxr-xr-x. 1 root      root      20 Jul 15 10:33 ..
> lrwxrwxrwx. 1 nfsnobody nfsnobody 54 Oct 10 13:21 .#test.txt -> jsynacek@jsynacek-ntb.brq.redhat.com.57671476081527
> -rw-rw-rw-+ 1 jsynacek  users     10 Oct 10 13:16 test.txt
> 
> When I save the file and quit emacs, the lock file stays there. It
> should be removed. Maybe it's because of the weird 0xF022 sequence in
> the file name?

That 0xF022 should have been a colon ':'.

Can you step with a debugger through lock_file_1 and see how this file
name happens to come up?  Is it possible that Samba doesn't allow the
colon characters in its file names, and the network re-director
replaces the colon with that strange character?  If so, would using a
different character, like comma, fix the problem?

Thanks.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Jan Synacek]

On Mon, Oct 10, 2016 at 2:14 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: jsynacek@redhat.com (Jan Synáček)
>> Date: Mon, 10 Oct 2016 13:30:24 +0200
>>
>> # ls -la /mnt/tmp/
>> total 9
>> drwxrwxrwx+ 2 root      root       0 Oct 10 13:21 .
>> drwxr-xr-x. 1 root      root      20 Jul 15 10:33 ..
>> lrwxrwxrwx. 1 nfsnobody nfsnobody 54 Oct 10 13:21 .#test.txt -> jsynacek@jsynacek-ntb.brq.redhat.com.57671476081527
>> -rw-rw-rw-+ 1 jsynacek  users     10 Oct 10 13:16 test.txt
>>
>> When I save the file and quit emacs, the lock file stays there. It
>> should be removed. Maybe it's because of the weird 0xF022 sequence in
>> the file name?
>
> That 0xF022 should have been a colon ':'.
>
> Can you step with a debugger through lock_file_1 and see how this file
> name happens to come up?  Is it possible that Samba doesn't allow the
> colon characters in its file names, and the network re-director
> replaces the colon with that strange character?  If so, would using a
> different character, like comma, fix the problem?
>
> Thanks.

The lock_file_1 correctly uses a path with a colon in it. Indeed,
samba seems to substitute it. I tried to patch the emacs source to use
a comma instead. It didn't get substituted by samba, but the lock file
didn't go away when I exited emacs.

-- 
Jan Synacek
Software Engineer, Red Hat

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> From: Jan Synacek <jsynacek@redhat.com>
> Date: Mon, 10 Oct 2016 15:49:11 +0200
> Cc: 24656@debbugs.gnu.org
> 
> The lock_file_1 correctly uses a path with a colon in it. Indeed,
> samba seems to substitute it. I tried to patch the emacs source to use
> a comma instead. It didn't get substituted by samba, but the lock file
> didn't go away when I exited emacs.

Did you also patch current_lock_owner to account for the change from a
colon to a comma?

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Jan Synacek]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Mon, Oct 10, 2016 at 5:49 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Jan Synacek <jsynacek@redhat.com>
>> Date: Mon, 10 Oct 2016 15:49:11 +0200
>> Cc: 24656@debbugs.gnu.org
>>
>> The lock_file_1 correctly uses a path with a colon in it. Indeed,
>> samba seems to substitute it. I tried to patch the emacs source to use
>> a comma instead. It didn't get substituted by samba, but the lock file
>> didn't go away when I exited emacs.
>
> Did you also patch current_lock_owner to account for the change from a
> colon to a comma?

Aha! After patching current_lock_owner, the lock file is correctly
removed. I'm attaching the patch.

Thank you!
-- 
Jan Synacek
Software Engineer, Red Hat

[-- Attachment #2: emacs.patch --]
[-- Type: text/x-patch, Size: 739 bytes --]

diff --git a/src/filelock.c b/src/filelock.c
index a2e1df9..bcc2e03 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -450,7 +450,7 @@ lock_file_1 (char *lfname, bool force)
     {
       if (sizeof lock_info_str
           <= snprintf (lock_info_str, sizeof lock_info_str,
-                       "%s@%s.%"pMd":%"pMd,
+                       "%s@%s.%"pMd",%"pMd,
                        user_name, host_name, pid, boot))
         return ENAMETOOLONG;
     }
@@ -564,7 +564,7 @@ current_lock_owner (lock_info_type *owner, char *lfname)
       lfinfo_end = owner->colon;
       break;
 
-    case ':':
+    case ',':
       if (! c_isdigit (owner->colon[1]))
 	return -1;
       boot_time = strtoimax (owner->colon + 1, &lfinfo_end, 10);

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> From: Jan Synacek <jsynacek@redhat.com>
> Date: Tue, 11 Oct 2016 08:32:35 +0200
> Cc: 24656@debbugs.gnu.org
> 
> > Did you also patch current_lock_owner to account for the change from a
> > colon to a comma?
> 
> Aha! After patching current_lock_owner, the lock file is correctly
> removed. I'm attaching the patch.
> 
> Thank you!

Thanks.

Paul, do you see any reasons not to make this change?  Could the comma
be problematic in some cases?  If so, can we try some other character,
perhaps?

If the change is okay, I'd like to make it on the emacs-25 branch.
Any objections?

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Jan Synacek]

On Tue, Oct 11, 2016 at 8:49 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Jan Synacek <jsynacek@redhat.com>
>> Date: Tue, 11 Oct 2016 08:32:35 +0200
>> Cc: 24656@debbugs.gnu.org
>>
>> > Did you also patch current_lock_owner to account for the change from a
>> > colon to a comma?
>>
>> Aha! After patching current_lock_owner, the lock file is correctly
>> removed. I'm attaching the patch.
>>
>> Thank you!
>
> Thanks.
>
> Paul, do you see any reasons not to make this change?  Could the comma
> be problematic in some cases?  If so, can we try some other character,
> perhaps?
>
> If the change is okay, I'd like to make it on the emacs-25 branch.
> Any objections?

One thing I noticed after creating the patch. You probably also want
to fix the comments that mention colon.

-- 
Jan Synacek
Software Engineer, Red Hat

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Paul Eggert]

On 10/10/2016 11:49 PM, Eli Zaretskii wrote:
> Paul, do you see any reasons not to make this change?  Could the comma
> be problematic in some cases?  If so, can we try some other character,
> perhaps?

If we're going to change it, I suggest choosing a character from the 
POSIX portable file name character set 
<http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266#tag_03_276>, 
as this is the most likely to work across a wide variety of POSIXish 
file systems. So, how about using "." instead of ":"?

More important, how will a new Emacs interoperate with an old one?

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> Cc: 24656@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 11 Oct 2016 08:59:41 -0700
> 
> On 10/10/2016 11:49 PM, Eli Zaretskii wrote:
> > Paul, do you see any reasons not to make this change?  Could the comma
> > be problematic in some cases?  If so, can we try some other character,
> > perhaps?
> 
> If we're going to change it, I suggest choosing a character from the 
> POSIX portable file name character set 
> <http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266#tag_03_276>, 
> as this is the most likely to work across a wide variety of POSIXish 
> file systems. So, how about using "." instead of ":"?

Fine with me, I thought we wanted a different character to minimize
the risk of errors in parsing the link name.

> More important, how will a new Emacs interoperate with an old one?

I guess we will have to check for both the colon and the new
character?

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Paul Eggert]

On 10/11/2016 09:29 AM, Eli Zaretskii wrote:
> I thought we wanted a different character to minimize
> the risk of errors in parsing the link name.

Ah, OK, perhaps it's due to worries about interoperating with an older 
Emacs scheme that used just "USER@DOMAIN" without the trailing 
".PID:BOOT". If so, we should use some string that cannot be interpreted 
as a valid domain name by an ancient Emacs. So, how about using '..' 
instead of ':'? That is, "USER@DOMAIN.PID..BOOT". This should work since 
a valid domain name cannot contain "..".

The complete set of allowed characters here, if '..' doesn't work for 
some reason, is '(', ')', '<', '>', '[', ']', ':', ';', '@', '\', ',', 
'.', '"', according to Internet RFC 5322 section 3.2.3. Of these 
characters only '.' is allowed in a POSIX portable file name.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> Cc: jsynacek@redhat.com, 24656@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 11 Oct 2016 09:53:40 -0700
> 
> On 10/11/2016 09:29 AM, Eli Zaretskii wrote:
> > I thought we wanted a different character to minimize
> > the risk of errors in parsing the link name.
> 
> Ah, OK, perhaps it's due to worries about interoperating with an older 
> Emacs scheme that used just "USER@DOMAIN" without the trailing 
> ".PID:BOOT". If so, we should use some string that cannot be interpreted 
> as a valid domain name by an ancient Emacs. So, how about using '..' 
> instead of ':'? That is, "USER@DOMAIN.PID..BOOT". This should work since 
> a valid domain name cannot contain "..".

Sounds fine to me.  I just tried this on a Windows filesystem, and it
allows that, so Samba should, too.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Achim Gratz]

Paul Eggert writes:
> On 10/10/2016 11:49 PM, Eli Zaretskii wrote:
>> Paul, do you see any reasons not to make this change?  Could the comma
>> be problematic in some cases?  If so, can we try some other character,
>> perhaps?
>
> If we're going to change it, I suggest choosing a character from the
> POSIX portable file name character set
> <http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266#tag_03_276>,
> as this is the most likely to work across a wide variety of POSIXish
> file systems. So, how about using "." instead of ":"?

You could maybe copy what Cygwin does if you need to handle wierdd characters in
file names:
https://cygwin.com/cygwin-ug-net/using-specialnames.html#pathnames-specialchars


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> From: Achim Gratz <Stromeko@nexgo.de>
> Date: Tue, 11 Oct 2016 19:43:08 +0200
> 
> You could maybe copy what Cygwin does if you need to handle wierdd characters in
> file names:
> https://cygwin.com/cygwin-ug-net/using-specialnames.html#pathnames-specialchars

Thanks.

That's what Samba seems to do, and that's the cause of this bug in the
first place.

We don't have the reason Cygwin does to deal with such problem, we can
simply use another character.  It's simpler and doesn't assume
anything about PUA codepoints or their support in the underlying
filesystem.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

On 10/11/2016 10:07 AM, Eli Zaretskii wrote:
> Sounds fine to me.  I just tried this on a Windows filesystem, and it
> allows that, so Samba should, too.

Unfortunately when I looked at the Emacs code more carefully, I found 
that '..' won't work because it will confuse older Emacs. So will '@'. 
Come to think of it, changing conventions in this area is likely to 
break interoperability not only with older Emacs, but also with other 
programs that attempt to work with Emacs's locking scheme.

I looked into Cygwin, and it seems to use U+F022 to represent '"', not 
':'. So this is not Cygwin-related.

I looked at the original Red Hat bug report, and found that I could 
reproduce the problem with a simple C program, i.e., without Emacs or 
Cygwin. It appears to be a Samba bug. I added a comment to that effect 
to the Red Hat bug report.

I installed into Emacs master the attached patch, which should work 
around the Samba bug without introducing so many interoperability 
hassles, and am boldly marking the Emacs bug as done. It's not clear to 
me whether this change is worth backporting into emacs-25, as Emacs has 
locked files this way for ages.

[-- Attachment #2: 0001-Work-around-Samba-bug-with-in-symlink-contents.patch --]
[-- Type: application/x-patch, Size: 3994 bytes --]

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> Cc: jsynacek@redhat.com, 24656-done@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 11 Oct 2016 13:15:50 -0700
> 
> I installed into Emacs master the attached patch, which should work 
> around the Samba bug without introducing so many interoperability 
> hassles, and am boldly marking the Emacs bug as done.

Thanks.

> It's not clear to me whether this change is worth backporting into
> emacs-25, as Emacs has locked files this way for ages.

I think we should have this in the next release, yes.  Do you see any
dangers in the patch that would make it prudent to give this change a
longer testing phase than what we will have before 25.2?

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Paul Eggert]

On 10/11/2016 01:58 PM, Eli Zaretskii wrote:
> Do you see any
> dangers in the patch that would make it prudent to give this change a
> longer testing phase than what we will have before 25.2?

No, the variant I installed into master should be pretty safe in that 
regard, as it avoids the interoperability issues inherent in instituting 
a new syntax, and the pattern it's looking for is unlikely to appear in 
scenarios that do not involve Emacs and this buggy file system.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> Cc: jsynacek@redhat.com, 24656@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 11 Oct 2016 15:46:58 -0700
> 
> On 10/11/2016 01:58 PM, Eli Zaretskii wrote:
> > Do you see any
> > dangers in the patch that would make it prudent to give this change a
> > longer testing phase than what we will have before 25.2?
> 
> No, the variant I installed into master should be pretty safe in that 
> regard, as it avoids the interoperability issues inherent in instituting 
> a new syntax, and the pattern it's looking for is unlikely to appear in 
> scenarios that do not involve Emacs and this buggy file system.

That was my impression as well, so I think it should be backported to
the release branch.

Thanks.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Paul Eggert]

On 10/11/2016 11:40 PM, Eli Zaretskii wrote:
> That was my impression as well, so I think it should be backported to
> the release branch.

OK, done. While I was at it, I fixed a problem that had been annoying me 
for some time, which is that './configure --enable-gcc-warnings' didn't 
work in emacs-25 with newer GCC.

bug#24656: 25.1; Emacs leaves lock files on a CIFS share

[Eli Zaretskii]

> Cc: jsynacek@redhat.com, 24656@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 12 Oct 2016 09:21:06 -0700
> 
> On 10/11/2016 11:40 PM, Eli Zaretskii wrote:
> > That was my impression as well, so I think it should be backported to
> > the release branch.
> 
> OK, done. While I was at it, I fixed a problem that had been annoying me 
> for some time, which is that './configure --enable-gcc-warnings' didn't 
> work in emacs-25 with newer GCC.

Thanks.