emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [BUG] Tangle with symbolic links don't work
@ 2023-11-06 16:08 Cletip Cletip
  2023-11-06 16:17 ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Cletip Cletip @ 2023-11-06 16:08 UTC (permalink / raw)
  To: Org Mode List

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-11-06 16:08 [BUG] Tangle with symbolic links don't work Cletip Cletip
@ 2023-11-06 16:17 ` Ihor Radchenko
  2023-11-07 10:55   ` Max Nikulin
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2023-11-06 16:17 UTC (permalink / raw)
  To: Cletip Cletip, Max Nikulin; +Cc: Org Mode List

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>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-11-06 16:17 ` Ihor Radchenko
@ 2023-11-07 10:55   ` Max Nikulin
  2023-11-07 11:30     ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Max Nikulin @ 2023-11-07 10:55 UTC (permalink / raw)
  To: Cletip Cletip; +Cc: Org Mode List

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-11-07 10:55   ` Max Nikulin
@ 2023-11-07 11:30     ` Ihor Radchenko
  2023-11-20  9:55       ` Cletip Cletip
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2023-11-07 11:30 UTC (permalink / raw)
  To: Max Nikulin; +Cc: Cletip Cletip, Org Mode List

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>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-11-07 11:30     ` Ihor Radchenko
@ 2023-11-20  9:55       ` Cletip Cletip
  2023-12-04 12:58         ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Cletip Cletip @ 2023-11-20  9:55 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Max Nikulin, Org Mode List

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-11-20  9:55       ` Cletip Cletip
@ 2023-12-04 12:58         ` Ihor Radchenko
  2023-12-15 11:26           ` Ihor Radchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Ihor Radchenko @ 2023-12-04 12:58 UTC (permalink / raw)
  To: Cletip Cletip; +Cc: Max Nikulin, Org Mode List

[-- 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>

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [BUG] Tangle with symbolic links don't work
  2023-12-04 12:58         ` Ihor Radchenko
@ 2023-12-15 11:26           ` Ihor Radchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Ihor Radchenko @ 2023-12-15 11:26 UTC (permalink / raw)
  To: Cletip Cletip; +Cc: Max Nikulin, Org Mode List

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>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-12-15 11:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-06 16:08 [BUG] Tangle with symbolic links don't work Cletip Cletip
2023-11-06 16:17 ` Ihor Radchenko
2023-11-07 10:55   ` Max Nikulin
2023-11-07 11:30     ` Ihor Radchenko
2023-11-20  9:55       ` Cletip Cletip
2023-12-04 12:58         ` Ihor Radchenko
2023-12-15 11:26           ` Ihor Radchenko

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).