[BUG] Tangle with symbolic links don't work

[BUG] Tangle with symbolic links don't work

[Cletip Cletip]

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

Hello everyone,

I'm reaching out to discuss a challenge I've encountered while working with
Org-mode, specifically during the tangling process of code blocks.

The Issue:
I have an Org file, test.org, from which I tangle code blocks into test.py.
The complication arises because test.py is a symbolic link. Each time I
perform the tangling operation, the symbolic link is unfortunately
overwritten.

Interestingly, when exporting documents to formats such as .tex, .html, or
others, Org-mode respects the symbolic link, which is the desired behavior.
However, this is not the case with tangling, which is quite perplexing.

My Org mode version : 9.6.10 (9.6.10-n/a-g902975...)

Proposed Solution:
I've identified a potential fix that involves a minor adjustment in the
ob-tangle.el file:

Original line (259):

(let ((file-name (car by-fn)))

Modified line:

(let ((file-name (file-truename (car by-fn))))

While this solution appears effective at first glance, I haven't
extensively tested all edge cases. I would greatly appreciate your insights
on whether this is a robust solution ^^.

Thanks in advance for your answer.

[-- Attachment #2: Type: text/html, Size: 1271 bytes --]

Re: [BUG] Tangle with symbolic links don't work

[Ihor Radchenko]

Cletip Cletip <clement020302@gmail.com> writes:

> The Issue:
> I have an Org file, test.org, from which I tangle code blocks into test.py.
> The complication arises because test.py is a symbolic link. Each time I
> perform the tangling operation, the symbolic link is unfortunately
> overwritten.
> ...
> Proposed Solution:
> I've identified a potential fix that involves a minor adjustment in the
> ob-tangle.el file:
>
> Original line (259):
>
> (let ((file-name (car by-fn)))
>
> Modified line:
>
> (let ((file-name (file-truename (car by-fn))))
>
> While this solution appears effective at first glance, I haven't
> extensively tested all edge cases. I would greatly appreciate your insights
> on whether this is a robust solution ^^.

Looks reasonable.
Max, do you see any pitfalls using `file-truename'?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Tangle with symbolic links don't work

[Max Nikulin]

On 06/11/2023 23:17, Ihor Radchenko wrote:
> Cletip Cletip writes:
> 
>> I have an Org file, test.org, from which I tangle code blocks into test.py.
>> The complication arises because test.py is a symbolic link.

I suggest to change your workflow to not depend on particular means to 
write tangled files. Perhaps you may set tangle directory. Unfortunately 
the description is too concise to figure out if it would be convenient 
enough.

> Max, do you see any pitfalls using `file-truename'?

Sorry, I am not familiar with related code path. That is why I can not 
reason what way to deal with file name is safer.

If there is a world-writable directory in the file path (usually 
$TMPDIR) then `file-truename' is less safe, see 
https://www.kernel.org/doc/html/latest/admin-guide/sysctl/fs.html#protected-symlinks

echo content >/home/ubuntu/tmp/file.txt
ls -l /home/ubuntu/tmp/file.txt
-rw-r--r-- 1 ubuntu ubuntu 8 Nov  7 17:29 /home/ubuntu/tmp/file.txt

ls -l /tmp/ln
lrwxrwxrwx 1 test test 25 Nov  7 17:10 /tmp/ln -> /home/ubuntu/tmp/file.txt

Notice different owner of the symlink.

echo overwrite >/tmp/ln
bash: /tmp/ln: Permission denied

cat /home/ubuntu/tmp/file.txt
content

`file-truename':

echo overwrite>"$(readlink -f /tmp/ln)"
cat /home/ubuntu/tmp/file.txt
overwrite

/usr/sbin/sysctl fs.protected_symlinks
fs.protected_symlinks = 1

In general, I am never sure that Org code follows best practices in 
respect to security in general and in respect to /tmp in particular. The 
following citation is unrelated to /tmp, but the same proposed patch has 
an issue with predictable name in /tmp:

Visuwesh… Re: [BUG] [PATCH] Add yank-media and DND handler. Wed, 27 Sep 
2023 13:59:49 +0530. https://list.orgmode.org/878r8sj9xe.fsf@gmail.com
> That would be quite annoying IMO.  I say we let the user shoot
> themselves in the foot.

Even when /tmp or similar directories are not involved, a proper 
strategy to replace file content should be carefully chosen. E.g. cp(1) 
preserves inode number while install(1) replaces target file atomically 
(create a temporary one and rename). The latter way is more suitable for 
shared libraries since it allows running application to continue call 
function from the deleted file.

I know, it is not an answer you expected from me, but giving a better 
one require to much efforts to read the code and to debug it.

Re: [BUG] Tangle with symbolic links don't work

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> Max, do you see any pitfalls using `file-truename'?
>
> Sorry, I am not familiar with related code path. That is why I can not 
> reason what way to deal with file name is safer.
>
> If there is a world-writable directory in the file path (usually 
> $TMPDIR) then `file-truename' is less safe, see 
> https://www.kernel.org/doc/html/latest/admin-guide/sysctl/fs.html#protected-symlinks

Thanks!

> In general, I am never sure that Org code follows best practices in 
> respect to security in general and in respect to /tmp in particular. The 
> following citation is unrelated to /tmp, but the same proposed patch has 
> an issue with predictable name in /tmp:

We have to compromise between usability and safety... but probably not
in this case.

> Even when /tmp or similar directories are not involved, a proper 
> strategy to replace file content should be carefully chosen. E.g. cp(1) 
> preserves inode number while install(1) replaces target file atomically 
> (create a temporary one and rename). The latter way is more suitable for 
> shared libraries since it allows running application to continue call 
> function from the deleted file.

What we actually use is Elisp API. For export and tangling, we use
`write-region' - it correctly handles TRAMP files with lower-level
details taken care of.

I can now see that blindly expanding to `file-truename' may not be wise.

Without `file-truename', the difference between ox.el (that works for
Cletip) and ob-tangle.el is that ob-tangle explicitly deletes the tangle
target before tangling:

`org-babel-tangle':

                     ;; erase previous file
                     (when (file-exists-p file-name)
                       (delete-file file-name))
		     (write-region nil nil file-name)
		     (mapc (lambda (mode) (set-file-modes file-name mode)) modes)

Rather than using `file-truename', we may instead remove the
`delete-file' part. This way, we will not risk changing file modes in
the original files and always modify the symlink, if the tangle target
is an existing symlink.

> I know, it is not an answer you expected from me, but giving a better 
> one require to much efforts to read the code and to debug it.

It is exactly an answer I expected, actually :)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Tangle with symbolic links don't work

[Cletip Cletip]

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

Hello !

Sorry to bring up the subject again, but I didn't quite understand what the
solution was: should I modify the function ? Is it modified in a new
version of org-mode ?

Thank you in advance for your response.

Le mar. 7 nov. 2023 à 12:28, Ihor Radchenko <yantar92@posteo.net> a écrit :

> Max Nikulin <manikulin@gmail.com> writes:
>
> >> Max, do you see any pitfalls using `file-truename'?
> >
> > Sorry, I am not familiar with related code path. That is why I can not
> > reason what way to deal with file name is safer.
> >
> > If there is a world-writable directory in the file path (usually
> > $TMPDIR) then `file-truename' is less safe, see
> >
> https://www.kernel.org/doc/html/latest/admin-guide/sysctl/fs.html#protected-symlinks
>
> Thanks!
>
> > In general, I am never sure that Org code follows best practices in
> > respect to security in general and in respect to /tmp in particular. The
> > following citation is unrelated to /tmp, but the same proposed patch has
> > an issue with predictable name in /tmp:
>
> We have to compromise between usability and safety... but probably not
> in this case.
>
> > Even when /tmp or similar directories are not involved, a proper
> > strategy to replace file content should be carefully chosen. E.g. cp(1)
> > preserves inode number while install(1) replaces target file atomically
> > (create a temporary one and rename). The latter way is more suitable for
> > shared libraries since it allows running application to continue call
> > function from the deleted file.
>
> What we actually use is Elisp API. For export and tangling, we use
> `write-region' - it correctly handles TRAMP files with lower-level
> details taken care of.
>
> I can now see that blindly expanding to `file-truename' may not be wise.
>
> Without `file-truename', the difference between ox.el (that works for
> Cletip) and ob-tangle.el is that ob-tangle explicitly deletes the tangle
> target before tangling:
>
> `org-babel-tangle':
>
>                      ;; erase previous file
>                      (when (file-exists-p file-name)
>                        (delete-file file-name))
>                      (write-region nil nil file-name)
>                      (mapc (lambda (mode) (set-file-modes file-name mode))
> modes)
>
> Rather than using `file-truename', we may instead remove the
> `delete-file' part. This way, we will not risk changing file modes in
> the original files and always modify the symlink, if the tangle target
> is an existing symlink.
>
> > I know, it is not an answer you expected from me, but giving a better
> > one require to much efforts to read the code and to debug it.
>
> It is exactly an answer I expected, actually :)
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>

[-- Attachment #2: Type: text/html, Size: 4066 bytes --]

Re: [BUG] Tangle with symbolic links don't work

[Ihor Radchenko]

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

Cletip Cletip <clement020302@gmail.com> writes:

> Sorry to bring up the subject again, but I didn't quite understand what the
> solution was: should I modify the function ? Is it modified in a new
> version of org-mode ?

The tentative solution is attached. May you test it?

The previous discussion was mostly about how to best approach the solution.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-babel-tangle-Do-note-erase-the-existing-tangle-t.patch --]
[-- Type: text/x-patch, Size: 1650 bytes --]

From cf9db7b5a602c2c0d4970c69a95d98640cefc9a0 Mon Sep 17 00:00:00 2001
Message-ID: <cf9db7b5a602c2c0d4970c69a95d98640cefc9a0.1701694627.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 4 Dec 2023 13:55:05 +0100
Subject: [PATCH] org-babel-tangle: Do note erase the existing tangle target
 before overwriting

* lisp/ob-tangle.el (org-babel-tangle): Do not remove the existing
tangle target file, if any.  `write-region' later will overwrite it
anyway, while removing may be unexpected if the existing target is a
symlink.

Reported-by: Cletip Cletip <clement020302@gmail.com>
Link: https://list.orgmode.org/orgmode/CAPHku6O9NfVMAfmE3_ahmpJea_2Qm0mJMFX6qPpT8uiQ94KMZA@mail.gmail.com/
---
 lisp/ob-tangle.el | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index b30fd9274..b48269897 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -310,9 +310,8 @@ (defun org-babel-tangle (&optional arg target-file lang-re)
                                       (compare-buffer-substrings
                                        nil nil nil
                                        tangle-buf nil nil)))))))
-                     ;; erase previous file
-                     (when (file-exists-p file-name)
-                       (delete-file file-name))
+                     ;; We do not erase, but overwrite previous file
+                     ;; to preserve any existing symlinks.
 		     (write-region nil nil file-name)
 		     (mapc (lambda (mode) (set-file-modes file-name mode)) modes))
                    (push file-name path-collector))))))
-- 
2.42.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Tangle with symbolic links don't work

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Subject: [PATCH] org-babel-tangle: Do note erase the existing tangle target
>  before overwriting

Applied.
Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=fbcd71e85

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>