Document that openlog's first paramater must continue to point to valid data

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Document that openlog's first paramater must continue to point to valid data

Laurence Tratt
OpenBSD's documentation for openlog's first paramater 'ident' is less clear
than Debian [1] or GNU [2] that the memory pointed to must remain valid for
as long as syslog is called (which I'm assuming without hard evidence is
equivalent to "until closelog is called").

Although this isn't specified by POSIX, on both OpenBSD and Debian passing a
pointer to memory that is then free'd causes random bytes to be written to
syslog. This tiny program demonstrates the problem:

  #include <stdlib.h>
  #include <string.h>
  #include <syslog.h>

  int main() {
      char *n = malloc(128);
      strcpy(n, "name");
      openlog(n, LOG_CONS, LOG_DAEMON);
      free(n);
      syslog(LOG_ERR, "msg");
      return 0;
  }

Most of the time this leads to rubbish being sent to /var/log/daemon: once
in a while it will segfault. Removing the free() call fixes the problem.

The patch at the end of this email is one possible suggestion for making this
clear, but it's difficult to do so succinctly.


Laurie

[1] https://manpages.debian.org/testing/manpages-dev/openlog.3.en.html
[2] https://www.gnu.org/software/libc/manual/html_node/openlog.html


Index: syslog.3
===================================================================
RCS file: /cvs/src/lib/libc/gen/syslog.3,v
retrieving revision 1.35
diff -u -r1.35 syslog.3
--- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
+++ syslog.3 2 Feb 2020 21:15:40 -0000
@@ -216,7 +216,17 @@
 .Fn vsyslog .
 The parameter
 .Fa ident
-is a string that will be prepended to every message.
+is a pointer to a string that will be prepended to every message. Note that
+.Fn openlog
+stores the
+.Fa ident
+pointer itself: it does not copy the string that
+.Fa ident
+points to, and does not guarantee safety if the contents of the string change
+later. You should thus ensure the memory pointed to by
+.Fa ident
+does not change until and unless you call
+.Fn closelog .
 The
 .Fa logopt
 argument

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Theo de Raadt-2
>OpenBSD's documentation for openlog's first paramater 'ident' is less clear
>than Debian [1] or GNU [2] that the memory pointed to must remain valid for
>as long as syslog is called (which I'm assuming without hard evidence is
>equivalent to "until closelog is called").
>
>Although this isn't specified by POSIX, on both OpenBSD and Debian passing a
>pointer to memory that is then free'd causes random bytes to be written to
>syslog. This tiny program demonstrates the problem:
>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <syslog.h>
>
>  int main() {
>      char *n = malloc(128);
>      strcpy(n, "name");
>      openlog(n, LOG_CONS, LOG_DAEMON);
>      free(n);
>      syslog(LOG_ERR, "msg");
>      return 0;
>  }

How did you arrive at writing code like this??

ident is a string, or pointer to some bytes.  It says that is used
by every syslog() call.  Why would you think to free it?  Why would
you assume it can be freed, where comes this assumption that openlog
would "save a copy of it"?  That is sheer madness.

>Most of the time this leads to rubbish being sent to /var/log/daemon: once
>in a while it will segfault. Removing the free() call fixes the problem.
>
>The patch at the end of this email is one possible suggestion for making this
>clear, but it's difficult to do so succinctly.

If we are going to document it, about 6 word adjustment speaking about
"storage", "lifetime", or "persisting" "until closelog()" should be enough,
You are falling into the trap of documenting what goes wrong rather than
how it works.

The parameter ident is a string that will be prepended to every
message, it's storage must persist until
Fn closelog .

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Laurence Tratt
On Sun, Feb 02, 2020 at 03:22:12PM -0700, Theo de Raadt wrote:

Hello Theo,

>> OpenBSD's documentation for openlog's first paramater 'ident' is less
>> clear than Debian [1] or GNU [2] that the memory pointed to must remain
>> valid for as long as syslog is called (which I'm assuming without hard
>> evidence is equivalent to "until closelog is called").
[...]
> If we are going to document it, about 6 word adjustment speaking about
> "storage", "lifetime", or "persisting" "until closelog()" should be enough,

Keeping it simple works for me. Patch at the bottom of this email.


Laurie


Index: syslog.3
===================================================================
RCS file: /cvs/src/lib/libc/gen/syslog.3,v
retrieving revision 1.35
diff -u -r1.35 syslog.3
--- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
+++ syslog.3 2 Feb 2020 23:13:30 -0000
@@ -216,7 +216,9 @@
 .Fn vsyslog .
 The parameter
 .Fa ident
-is a string that will be prepended to every message.
+is a string that will be prepended to every message; both storage and contents
+must persist unchanged until
+.Fn closelog .
 The
 .Fa logopt
 argument

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Theo de Raadt-2
Laurence Tratt <[hidden email]> wrote:

> On Sun, Feb 02, 2020 at 03:22:12PM -0700, Theo de Raadt wrote:
>
> Hello Theo,
>
> >> OpenBSD's documentation for openlog's first paramater 'ident' is less
> >> clear than Debian [1] or GNU [2] that the memory pointed to must remain
> >> valid for as long as syslog is called (which I'm assuming without hard
> >> evidence is equivalent to "until closelog is called").
> [...]
> > If we are going to document it, about 6 word adjustment speaking about
> > "storage", "lifetime", or "persisting" "until closelog()" should be enough,
>
> Keeping it simple works for me. Patch at the bottom of this email.
>
>
> Laurie
>
>
> Index: syslog.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/syslog.3,v
> retrieving revision 1.35
> diff -u -r1.35 syslog.3
> --- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
> +++ syslog.3 2 Feb 2020 23:13:30 -0000
> @@ -216,7 +216,9 @@
>  .Fn vsyslog .
>  The parameter
>  .Fa ident
> -is a string that will be prepended to every message.
> +is a string that will be prepended to every message; both storage and contents
> +must persist unchanged until
> +.Fn closelog .
>  The
>  .Fa logopt
>  argument

What is the difference between storage and contents

(actually contents could change.  As long as there is always a terminating NUL,
for the purpose of traversal.  But that's not something someone should code to)

You still didn't say how you ran into this.

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Laurence Tratt
On Sun, Feb 02, 2020 at 04:20:13PM -0700, Theo de Raadt wrote:

Hello Theo,

> What is the difference between storage and contents

You can't free() the backing memory *and* (according to GNU's man page, at
least) you can't safely change the string contents either on some OSs. I've
tried something slightly lengthier in the patch below.

> (actually contents could change.  As long as there is always a terminating
> NUL, for the purpose of traversal.  But that's not something someone should
> code to)
>
> You still didn't say how you ran into this.

While writing a daemon in Rust, I discovered that, AFAICT, one doesn't have
sensible access to __progname and has to figure out the executable name
dynamically (AFAICT), so it ends up in malloc'd storage. When I passed that
to openlog, and later called syslog, well, I was confused. I then distilled
the example down to C. The Debian & GNU man pages suggest that I'm not the
first person to try doing this.


Laurie


Index: syslog.3
===================================================================
RCS file: /cvs/src/lib/libc/gen/syslog.3,v
retrieving revision 1.35
diff -u -r1.35 syslog.3
--- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
+++ syslog.3 2 Feb 2020 23:32:50 -0000
@@ -216,7 +216,9 @@
 .Fn vsyslog .
 The parameter
 .Fa ident
-is a string that will be prepended to every message.
+is a string that will be prepended to every message; the memory pointed to
+must remain valid, and the string's contents unchanged, until
+.Fn closelog .
 The
 .Fa logopt
 argument

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Theo de Raadt-2
Laurence Tratt <[hidden email]> wrote:

> On Sun, Feb 02, 2020 at 04:20:13PM -0700, Theo de Raadt wrote:
>
> Hello Theo,
>
> > What is the difference between storage and contents
>
> You can't free() the backing memory *and* (according to GNU's man page, at
> least) you can't safely change the string contents either on some OSs. I've
> tried something slightly lengthier in the patch below.
>
> > (actually contents could change.  As long as there is always a terminating
> > NUL, for the purpose of traversal.  But that's not something someone should
> > code to)
> >
> > You still didn't say how you ran into this.
>
> While writing a daemon in Rust, I discovered that, AFAICT, one doesn't have
> sensible access to __progname

Pretty ignorant of them.

> and has to figure out the executable name
> dynamically (AFAICT),

That is impossible on many systems, or possible ... but can fail to deliver
a name and then what?

> so it ends up in malloc'd storage.

yes so, why was it being freed?

> When I passed that to openlog, and later called syslog, well, I was
> confused. I then distilled the example down to C. The Debian & GNU man
> pages suggest that I'm not the first person to try doing this.

That would only be true if you've looked at the history of how that
text ended up in their pages, as it is, I think either way overspecifies it,
I think openlog() could plausibly be coded to cache a copy of the whole
string.  The standard may have left this open-ended intentionally.
Is there a historian watching this thread?

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Laurence Tratt
On Sun, Feb 02, 2020 at 04:45:53PM -0700, Theo de Raadt wrote:

Hello Theo,

>> When I passed that to openlog, and later called syslog, well, I was
>> confused. I then distilled the example down to C. The Debian & GNU man
>> pages suggest that I'm not the first person to try doing this.
> That would only be true if you've looked at the history of how that text
> ended up in their pages, as it is, I think either way overspecifies it, I
> think openlog() could plausibly be coded to cache a copy of the whole
> string.  The standard may have left this open-ended intentionally. Is there
> a historian watching this thread?

I wasn't able to find anything out when I looked into this a bit, but I'm not
a Posix or GNU historian, so I might have missed something obvious.

The good news is that the altered text I'm suggesting would still guarantee
identical observed behaviour should a given syslog implementation choose to
copy 'ident' into a buffer.


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Ingo Schwarze
In reply to this post by Theo de Raadt-2
Hi,

since our manual page doesn't explain the details of how openlog(3)
uses *ident, it seems reasonable for users to conclude that it is
safest to neither free nor modify it.

Then again, given that in our implementation, freeing it may even
pose a security hazard, i might seem friendly to give more details.

POSIX has the same wording as our manual page (and so do 4.4BSD-Lite2,
FreeBSD, NetBSD, and Oracle Solaris 11).  As far as i understand,
the wording being in POSIX implies behaviour is unspecified if the
memory becomes invalid or if its content is changed.  That's also
how the Linux man-pages project documents it.  I don't think
researching history is needed for this; knowing that it's unspecified
feels sufficient.

Given that our implementation chooses to use-after-free (as it is
permitted to) if the memory becomes invalid, i prefer the Theo's
strong wording "must persist" to the possibly less discouraging
"unspecified" - foremost, we are documenting *our* implementation.

Regarding changes of the content, i consider it friendly to mention
that it is unspecified - otherwise, people might mistakenly assume
that our behaviour were required by POSIX.

While here, add the missing pointer to POSIX, correct HISTORY,
drop redundant verbiage from RETURN VALUES, and garbage collect .Tn.
Admittedly, that's more than one change in one patch, but all of
it is fairly standard, so why waste time splitting it.
(Still, feel free to OK only parts, of course.)

OK?
  Ingo


Index: syslog.3
===================================================================
RCS file: /cvs/src/lib/libc/gen/syslog.3,v
retrieving revision 1.35
diff -u -r1.35 syslog.3
--- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
+++ syslog.3 3 Feb 2020 20:16:39 -0000
@@ -216,12 +216,17 @@
 .Fn vsyslog .
 The parameter
 .Fa ident
-is a string that will be prepended to every message.
+points to a string that will be prepended to every message;
+its storage must persist until
+.Fn closelog
+or the corresponding
+.Fn closelog_r .
+If the content of the string is changed, behaviour is unspecified.
+.Pp
 The
 .Fa logopt
 argument
-is a bit field specifying logging options, which is formed by
-.Tn OR Ns 'ing
+is a bit field specifying logging options, which is formed by OR'ing
 one or more of the following values:
 .Bl -tag -width LOG_AUTHPRIV
 .It Dv LOG_CONS
@@ -310,18 +315,6 @@
 .Fa syslog_data
 structure.
 .Sh RETURN VALUES
-The
-.Fn closelog ,
-.Fn closelog_r ,
-.Fn openlog ,
-.Fn openlog_r ,
-.Fn syslog ,
-.Fn syslog_r ,
-.Fn vsyslog ,
-and
-.Fn vsyslog_r
-functions return no value.
-.Pp
 The routines
 .Fn setlogmask
 and
@@ -349,11 +342,58 @@
 .Sh SEE ALSO
 .Xr logger 1 ,
 .Xr syslogd 8
+.Sh STANDARDS
+The functions
+.Fn syslog ,
+.Fn openlog ,
+.Fn closelog ,
+and
+.Fn setlogmask
+conform to the X/Open Systems Interfaces option of
+.St -p1003.1-2008 .
+.Pp
+The facilities
+.Dv LOG_AUTHPRIV ,
+.Dv LOG_FTP ,
+and
+.Dv LOG_SYSLOG ,
+the option
+.Dv LOG_PERROR ,
+and the macro
+.Fn LOG_UPTO
+are extensions to that standard.
+.Pp
+The standard option
+.Dv LOG_NOWAIT
+is deprecated in
+.Ox
+and has no effect.
 .Sh HISTORY
-These
-functions appeared in
-.Bx 4.2 .
-The reentrant functions appeared in
+The functions
+.Fn syslog ,
+.Fn openlog ,
+and
+.Fn closelog
+appeared in
+.Bx 4.2 ,
+.Fn setlogmask
+in
+.Bx 4.3 ,
+and
+.Fn vsyslog
+in
+.Bx 4.3 Net/1 .
+.Pp
+The functions
+.Fn syslog_r ,
+.Fn vsyslog_r ,
+.Fn openlog_r ,
+.Fn closelog_r ,
+and
+.Fn setlogmask_r
+appeared in
+.Bx 386 0.1
+and have been available since
 .Ox 3.1 .
 .Sh CAVEATS
 It is important never to pass a string with user-supplied data as a

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Andreas Kusalananda Kähäri-4
On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:

> Hi,
>
> since our manual page doesn't explain the details of how openlog(3)
> uses *ident, it seems reasonable for users to conclude that it is
> safest to neither free nor modify it.
>
> Then again, given that in our implementation, freeing it may even
> pose a security hazard, i might seem friendly to give more details.
>
> POSIX has the same wording as our manual page (and so do 4.4BSD-Lite2,
> FreeBSD, NetBSD, and Oracle Solaris 11).  As far as i understand,
> the wording being in POSIX implies behaviour is unspecified if the
> memory becomes invalid or if its content is changed.  That's also
> how the Linux man-pages project documents it.  I don't think
> researching history is needed for this; knowing that it's unspecified
> feels sufficient.
>
> Given that our implementation chooses to use-after-free (as it is
> permitted to) if the memory becomes invalid, i prefer the Theo's
> strong wording "must persist" to the possibly less discouraging
> "unspecified" - foremost, we are documenting *our* implementation.
>
> Regarding changes of the content, i consider it friendly to mention
> that it is unspecified - otherwise, people might mistakenly assume
> that our behaviour were required by POSIX.
>
> While here, add the missing pointer to POSIX, correct HISTORY,
> drop redundant verbiage from RETURN VALUES, and garbage collect .Tn.
> Admittedly, that's more than one change in one patch, but all of
> it is fairly standard, so why waste time splitting it.
> (Still, feel free to OK only parts, of course.)

For what it's worth:

Related: https://www.austingroupbugs.net/view.php?id=1244

A proposal seems to have been accepted (if I'm reading it correctly) in
November of last year to change the wording in POSIX from

        The ident argument is a string that is prepended to every
        message.

to

        The ident argument is a pointer to a null-terminated identifier
        that shall be prepended (without the null terminator) to every
        message. The application shall ensure that the string pointed
        to by ident remains valid during the syslog() calls that will
        prepend this identifier; however, it is unspecified whether
        changes made to the string will change the identifier prepended
        by later syslog() calls.


Regards,

--
Andreas (Kusalananda) Kähäri
SciLifeLab, NBIS, ICM
Uppsala University, Sweden

.

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Ingo Schwarze
Hi Andreas,

Andreas Kusalananda wrote on Mon, Feb 03, 2020 at 09:56:22PM +0100:

> Related: https://www.austingroupbugs.net/view.php?id=1244
>
> A proposal seems to have been accepted (if I'm reading it correctly) in
> November of last year to change the wording in POSIX from
>
>         The ident argument is a string that is prepended to every
>         message.
>
> to
>
>         The ident argument is a pointer to a null-terminated identifier
>         that shall be prepended (without the null terminator) to every
>         message. The application shall ensure that the string pointed
>         to by ident remains valid during the syslog() calls that will
>         prepend this identifier; however, it is unspecified whether
>         changes made to the string will change the identifier prepended
>         by later syslog() calls.

Thanks for finding and showing the ticket.

That means the Austin Group agreed with the way i read the standard,
issued an interpretation clarifying it, and my patch is accurate in
that respect.

Note that we generally use simpler wording in our manual pages
than the Austin Group uses in the standard, to make the manuals
more readable.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Jason McIntyre-2
In reply to this post by Ingo Schwarze
On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:

> Hi,
>
> since our manual page doesn't explain the details of how openlog(3)
> uses *ident, it seems reasonable for users to conclude that it is
> safest to neither free nor modify it.
>
> Then again, given that in our implementation, freeing it may even
> pose a security hazard, i might seem friendly to give more details.
>
> POSIX has the same wording as our manual page (and so do 4.4BSD-Lite2,
> FreeBSD, NetBSD, and Oracle Solaris 11).  As far as i understand,
> the wording being in POSIX implies behaviour is unspecified if the
> memory becomes invalid or if its content is changed.  That's also
> how the Linux man-pages project documents it.  I don't think
> researching history is needed for this; knowing that it's unspecified
> feels sufficient.
>
> Given that our implementation chooses to use-after-free (as it is
> permitted to) if the memory becomes invalid, i prefer the Theo's
> strong wording "must persist" to the possibly less discouraging
> "unspecified" - foremost, we are documenting *our* implementation.
>
> Regarding changes of the content, i consider it friendly to mention
> that it is unspecified - otherwise, people might mistakenly assume
> that our behaviour were required by POSIX.
>
> While here, add the missing pointer to POSIX, correct HISTORY,
> drop redundant verbiage from RETURN VALUES, and garbage collect .Tn.
> Admittedly, that's more than one change in one patch, but all of
> it is fairly standard, so why waste time splitting it.
> (Still, feel free to OK only parts, of course.)
>
> OK?
>   Ingo
>

hi.

it reads ok, but i can't say a great deal more than that.

i don;t want to quibble about the wording, but i'll say it anyway:

- "must" reads badly. i understand this may be the exact word you want,
  but i'm not a fan

- the sentence structure is slighly unbalanced. i'll explain inline:

>
> Index: syslog.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/syslog.3,v
> retrieving revision 1.35
> diff -u -r1.35 syslog.3
> --- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
> +++ syslog.3 3 Feb 2020 20:16:39 -0000
> @@ -216,12 +216,17 @@
>  .Fn vsyslog .
>  The parameter
>  .Fa ident
> -is a string that will be prepended to every message.
> +points to a string that will be prepended to every message;
> +its storage must persist until
> +.Fn closelog
> +or the corresponding
> +.Fn closelog_r .
> +If the content of the string is changed, behaviour is unspecified.


so it's like this now:

        This is what ident is;
        here i append some info about not changing it.
        Now i start a new sentence to add more info about not changing it.

i guess i would write it sth like:

        The parameter
        .Fa ident
        points to a string that is prepended to every message.
        It should not be changed until either
        .Fn closelog
        or
        .Fn closelog_r
        are called,
        otherwise the behaviour is unspecified.

well, you get the gist.
jmc

> +.Pp
>  The
>  .Fa logopt
>  argument
> -is a bit field specifying logging options, which is formed by
> -.Tn OR Ns 'ing
> +is a bit field specifying logging options, which is formed by OR'ing
>  one or more of the following values:
>  .Bl -tag -width LOG_AUTHPRIV
>  .It Dv LOG_CONS
> @@ -310,18 +315,6 @@
>  .Fa syslog_data
>  structure.
>  .Sh RETURN VALUES
> -The
> -.Fn closelog ,
> -.Fn closelog_r ,
> -.Fn openlog ,
> -.Fn openlog_r ,
> -.Fn syslog ,
> -.Fn syslog_r ,
> -.Fn vsyslog ,
> -and
> -.Fn vsyslog_r
> -functions return no value.
> -.Pp
>  The routines
>  .Fn setlogmask
>  and
> @@ -349,11 +342,58 @@
>  .Sh SEE ALSO
>  .Xr logger 1 ,
>  .Xr syslogd 8
> +.Sh STANDARDS
> +The functions
> +.Fn syslog ,
> +.Fn openlog ,
> +.Fn closelog ,
> +and
> +.Fn setlogmask
> +conform to the X/Open Systems Interfaces option of
> +.St -p1003.1-2008 .
> +.Pp
> +The facilities
> +.Dv LOG_AUTHPRIV ,
> +.Dv LOG_FTP ,
> +and
> +.Dv LOG_SYSLOG ,
> +the option
> +.Dv LOG_PERROR ,
> +and the macro
> +.Fn LOG_UPTO
> +are extensions to that standard.
> +.Pp
> +The standard option
> +.Dv LOG_NOWAIT
> +is deprecated in
> +.Ox
> +and has no effect.
>  .Sh HISTORY
> -These
> -functions appeared in
> -.Bx 4.2 .
> -The reentrant functions appeared in
> +The functions
> +.Fn syslog ,
> +.Fn openlog ,
> +and
> +.Fn closelog
> +appeared in
> +.Bx 4.2 ,
> +.Fn setlogmask
> +in
> +.Bx 4.3 ,
> +and
> +.Fn vsyslog
> +in
> +.Bx 4.3 Net/1 .
> +.Pp
> +The functions
> +.Fn syslog_r ,
> +.Fn vsyslog_r ,
> +.Fn openlog_r ,
> +.Fn closelog_r ,
> +and
> +.Fn setlogmask_r
> +appeared in
> +.Bx 386 0.1
> +and have been available since
>  .Ox 3.1 .
>  .Sh CAVEATS
>  It is important never to pass a string with user-supplied data as a

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Stuart Henderson
On 2020/02/03 21:40, Jason McIntyre wrote:

> On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:
> > Hi,
> >
> > since our manual page doesn't explain the details of how openlog(3)
> > uses *ident, it seems reasonable for users to conclude that it is
> > safest to neither free nor modify it.
> >
> > Then again, given that in our implementation, freeing it may even
> > pose a security hazard, i might seem friendly to give more details.
> >
> > POSIX has the same wording as our manual page (and so do 4.4BSD-Lite2,
> > FreeBSD, NetBSD, and Oracle Solaris 11).  As far as i understand,
> > the wording being in POSIX implies behaviour is unspecified if the
> > memory becomes invalid or if its content is changed.  That's also
> > how the Linux man-pages project documents it.  I don't think
> > researching history is needed for this; knowing that it's unspecified
> > feels sufficient.
> >
> > Given that our implementation chooses to use-after-free (as it is
> > permitted to) if the memory becomes invalid, i prefer the Theo's
> > strong wording "must persist" to the possibly less discouraging
> > "unspecified" - foremost, we are documenting *our* implementation.
> >
> > Regarding changes of the content, i consider it friendly to mention
> > that it is unspecified - otherwise, people might mistakenly assume
> > that our behaviour were required by POSIX.
> >
> > While here, add the missing pointer to POSIX, correct HISTORY,
> > drop redundant verbiage from RETURN VALUES, and garbage collect .Tn.
> > Admittedly, that's more than one change in one patch, but all of
> > it is fairly standard, so why waste time splitting it.
> > (Still, feel free to OK only parts, of course.)
> >
> > OK?
> >   Ingo
> >
>
> hi.
>
> it reads ok, but i can't say a great deal more than that.
>
> i don;t want to quibble about the wording, but i'll say it anyway:
>
> - "must" reads badly. i understand this may be the exact word you want,
>   but i'm not a fan
>
> - the sentence structure is slighly unbalanced. i'll explain inline:
>
> >
> > Index: syslog.3
> > ===================================================================
> > RCS file: /cvs/src/lib/libc/gen/syslog.3,v
> > retrieving revision 1.35
> > diff -u -r1.35 syslog.3
> > --- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
> > +++ syslog.3 3 Feb 2020 20:16:39 -0000
> > @@ -216,12 +216,17 @@
> >  .Fn vsyslog .
> >  The parameter
> >  .Fa ident
> > -is a string that will be prepended to every message.
> > +points to a string that will be prepended to every message;
> > +its storage must persist until
> > +.Fn closelog
> > +or the corresponding
> > +.Fn closelog_r .
> > +If the content of the string is changed, behaviour is unspecified.
>
>
> so it's like this now:
>
> This is what ident is;
> here i append some info about not changing it.
> Now i start a new sentence to add more info about not changing it.
>
> i guess i would write it sth like:
>
> The parameter
> .Fa ident
> points to a string that is prepended to every message.
> It should not be changed until either
> .Fn closelog
> or
> .Fn closelog_r
> are called,
> otherwise the behaviour is unspecified.
>
> well, you get the gist.
> jmc

The "unspecified behaviour" relates to changing the contents of the
string after calling openlog.

If the storage of the string does not persist (i.e. if it is freed)
then it will definitely fail, in a bad way.

My attempt at changing your wording to add this information back results
in it turning into pretty much the same as Ingo's (but with slightly
worse choice of words).

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Jason McIntyre-2
On Mon, Feb 03, 2020 at 09:53:39PM +0000, Stuart Henderson wrote:

> On 2020/02/03 21:40, Jason McIntyre wrote:
> > On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:
> > > Hi,
> > >
> > > since our manual page doesn't explain the details of how openlog(3)
> > > uses *ident, it seems reasonable for users to conclude that it is
> > > safest to neither free nor modify it.
> > >
> > > Then again, given that in our implementation, freeing it may even
> > > pose a security hazard, i might seem friendly to give more details.
> > >
> > > POSIX has the same wording as our manual page (and so do 4.4BSD-Lite2,
> > > FreeBSD, NetBSD, and Oracle Solaris 11).  As far as i understand,
> > > the wording being in POSIX implies behaviour is unspecified if the
> > > memory becomes invalid or if its content is changed.  That's also
> > > how the Linux man-pages project documents it.  I don't think
> > > researching history is needed for this; knowing that it's unspecified
> > > feels sufficient.
> > >
> > > Given that our implementation chooses to use-after-free (as it is
> > > permitted to) if the memory becomes invalid, i prefer the Theo's
> > > strong wording "must persist" to the possibly less discouraging
> > > "unspecified" - foremost, we are documenting *our* implementation.
> > >
> > > Regarding changes of the content, i consider it friendly to mention
> > > that it is unspecified - otherwise, people might mistakenly assume
> > > that our behaviour were required by POSIX.
> > >
> > > While here, add the missing pointer to POSIX, correct HISTORY,
> > > drop redundant verbiage from RETURN VALUES, and garbage collect .Tn.
> > > Admittedly, that's more than one change in one patch, but all of
> > > it is fairly standard, so why waste time splitting it.
> > > (Still, feel free to OK only parts, of course.)
> > >
> > > OK?
> > >   Ingo
> > >
> >
> > hi.
> >
> > it reads ok, but i can't say a great deal more than that.
> >
> > i don;t want to quibble about the wording, but i'll say it anyway:
> >
> > - "must" reads badly. i understand this may be the exact word you want,
> >   but i'm not a fan
> >
> > - the sentence structure is slighly unbalanced. i'll explain inline:
> >
> > >
> > > Index: syslog.3
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/syslog.3,v
> > > retrieving revision 1.35
> > > diff -u -r1.35 syslog.3
> > > --- syslog.3 30 Aug 2019 20:27:25 -0000 1.35
> > > +++ syslog.3 3 Feb 2020 20:16:39 -0000
> > > @@ -216,12 +216,17 @@
> > >  .Fn vsyslog .
> > >  The parameter
> > >  .Fa ident
> > > -is a string that will be prepended to every message.
> > > +points to a string that will be prepended to every message;
> > > +its storage must persist until
> > > +.Fn closelog
> > > +or the corresponding
> > > +.Fn closelog_r .
> > > +If the content of the string is changed, behaviour is unspecified.
> >
> >
> > so it's like this now:
> >
> > This is what ident is;
> > here i append some info about not changing it.
> > Now i start a new sentence to add more info about not changing it.
> >
> > i guess i would write it sth like:
> >
> > The parameter
> > .Fa ident
> > points to a string that is prepended to every message.
> > It should not be changed until either
> > .Fn closelog
> > or
> > .Fn closelog_r
> > are called,
> > otherwise the behaviour is unspecified.
> >
> > well, you get the gist.
> > jmc
>
> The "unspecified behaviour" relates to changing the contents of the
> string after calling openlog.
>
> If the storage of the string does not persist (i.e. if it is freed)
> then it will definitely fail, in a bad way.
>
> My attempt at changing your wording to add this information back results
> in it turning into pretty much the same as Ingo's (but with slightly
> worse choice of words).
>

heh, i see. when i said:
> > it reads ok, but i can't say a great deal more than that.

i was referring to my lack of knowledge about the issue!

so i guess i'm fine with ingo's text.

jmc

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Theo de Raadt-2
In reply to this post by Ingo Schwarze
> +If the content of the string is changed, behaviour is unspecified.

I like that.  Since it is unspecified whether openlog stores the pointer
or the string, it should discourage people from trying to be clever.

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Todd C. Miller-3
In reply to this post by Ingo Schwarze
On Mon, 03 Feb 2020 21:28:15 +0100, Ingo Schwarze wrote:

> Given that our implementation chooses to use-after-free (as it is
> permitted to) if the memory becomes invalid, i prefer the Theo's
> strong wording "must persist" to the possibly less discouraging
> "unspecified" - foremost, we are documenting *our* implementation.
>
> Regarding changes of the content, i consider it friendly to mention
> that it is unspecified - otherwise, people might mistakenly assume
> that our behaviour were required by POSIX.

This reads fine to me.  We call out a similar issue in putenv(3).

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Ossi Herrala-4
In reply to this post by Laurence Tratt
On Sun, Feb 02, 2020 at 11:37:12PM +0000, Laurence Tratt wrote:

> While writing a daemon in Rust, I discovered that, AFAICT, one doesn't have
> sensible access to __progname and has to figure out the executable name
> dynamically (AFAICT), so it ends up in malloc'd storage.
>

You probably should use getprogname(3) directly or via libc
crate. Apparently getprogname(3) is just "return __progname":

  https://github.com/openbsd/src/blob/master/lib/libc/gen/getprogname.c

getprogname(3) is supported at least in OpenBSD, NetBSD, FreeBSD,
DragonBSD, macOS and Apple iOS.

So something like this should work. There's also some gymnastics from
null terminated string to Rust string but as long as you keep it as
CStr no allocation should occur.


use std::ffi::CStr;
use std::os::raw::c_char;

extern "C" {
    pub fn getprogname() -> *const c_char;
}

fn main() {
    let progname = unsafe {
        let ptr = getprogname();
        CStr::from_ptr(ptr)
    };

    // Lossy conversion to UTF-8 string so we can print it out stdout
    let name = progname.to_string_lossy();
    println!("progname: {}", name);
}


--
Ossi Herrala

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Laurence Tratt
In reply to this post by Ingo Schwarze
On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:

Hello Ingo,

>  The parameter
>  .Fa ident
> -is a string that will be prepended to every message.
> +points to a string that will be prepended to every message;
> +its storage must persist until
> +.Fn closelog
> +or the corresponding
> +.Fn closelog_r .
> +If the content of the string is changed, behaviour is unspecified.

This works well for me and would, I think, have stopped me making the
mistake that started this whole thread -- thanks!


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Document that openlog's first paramater must continue to point to valid data

Ingo Schwarze
Hi,

Laurence Tratt wrote on Wed, Feb 05, 2020 at 09:38:28AM +0000:
> On Mon, Feb 03, 2020 at 09:28:15PM +0100, Ingo Schwarze wrote:

>>  The parameter
>>  .Fa ident
>> -is a string that will be prepended to every message.
>> +points to a string that will be prepended to every message;
>> +its storage must persist until
>> +.Fn closelog
>> +or the corresponding
>> +.Fn closelog_r .
>> +If the content of the string is changed, behaviour is unspecified.

> This works well for me and would, I think, have stopped me making the
> mistake that started this whole thread -- thanks!

Committed.  Thanks for bringing it up a for the cross-check,
and to everybody for the feedback.

Yours,
  Ingo