[pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Will Page]

The fcntl guts switch on "cmd" parameter to identify the fcntl
command being issued, but isn't aware of the file creation flags that
can be ORed in.

This change masks out the flags from the command only for the switch
statement so that it can correctly determine whether it needs to pass
"lock" or "arg".  When real_fcntl() is called, the original value is
still passed on. This corrects an issue observed using pseudo on modern
linux desktops resulting in "pseudo: unknown fcntl argument 1030,
assuming long argument." diagnostics in the output.  Decimal 1030 is
0x0406, which translates to O_NOCTTY | F_SETLK, and should call
"rc = real_fcntl(fd, cmd, lock);", but instead falls through to the
default case instead calling "rc = real_fcntl(fd, cmd, arg);".

Tested the change using perftest - without this patch, the issue is
trivially reproducible on my Ubuntu Xenial system, and after patching it
emitted no "unknown fcntl argument" diagnostics.

Signed-off-by: Will Page <Will.Page@ni.com>
---
 ports/linux/guts/fcntl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ports/linux/guts/fcntl.c b/ports/linux/guts/fcntl.c
index 639fd24..d278a8c 100644
--- a/ports/linux/guts/fcntl.c
+++ b/ports/linux/guts/fcntl.c
@@ -8,6 +8,10 @@
  */
  	long arg;
 	int save_errno;
+	/* some bits can be ORed into the cmd should be explicitly ignored
+	 * see fcntl documentation on F_SETFL */
+	int o_mode_mask = (O_RDONLY | O_WRONLY | O_RDWR) |
+		(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	/* we don't know whether we need lock or arg; grab both, which
 	 * should be safe enough on Linuxy systems. */
@@ -15,7 +19,7 @@
 	arg = va_arg(ap, long);
 	va_end(ap);
 
-	switch (cmd) {
+	switch (cmd & ~(o_mode_mask)) {
 	case F_DUPFD:
 #ifdef F_DUPFD_CLOEXEC
 	case F_DUPFD_CLOEXEC:
-- 
2.7.4

✗ patchtest: failure for Fix to fcntl guts to ignore flags that can be ORed into cmd

[Patchwork]

== Series Details ==

Series: Fix to fcntl guts to ignore flags that can be ORed into cmd
Revision: 1
URL   : https://patchwork.openembedded.org/series/8950/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Patch            [pseudo] Fix to fcntl guts to ignore flags that can be ORed into cmd
 Issue             Shortlog does not follow expected format [test_shortlog_format] 
  Suggested fix    Commit shortlog (first line of commit message) should follow the format "<target>: <summary>"

* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at d72d116e02)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Burton, Ross]

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

Hi Will,

This should go to the yocto@yoctoproject.org list really.  I've CC'd the
maintainer to be sure he spots it.

Ross

On 15 September 2017 at 23:27, Will Page <Will.Page@ni.com> wrote:

> The fcntl guts switch on "cmd" parameter to identify the fcntl
> command being issued, but isn't aware of the file creation flags that
> can be ORed in.
>
> This change masks out the flags from the command only for the switch
> statement so that it can correctly determine whether it needs to pass
> "lock" or "arg".  When real_fcntl() is called, the original value is
> still passed on. This corrects an issue observed using pseudo on modern
> linux desktops resulting in "pseudo: unknown fcntl argument 1030,
> assuming long argument." diagnostics in the output.  Decimal 1030 is
> 0x0406, which translates to O_NOCTTY | F_SETLK, and should call
> "rc = real_fcntl(fd, cmd, lock);", but instead falls through to the
> default case instead calling "rc = real_fcntl(fd, cmd, arg);".
>
> Tested the change using perftest - without this patch, the issue is
> trivially reproducible on my Ubuntu Xenial system, and after patching it
> emitted no "unknown fcntl argument" diagnostics.
>
> Signed-off-by: Will Page <Will.Page@ni.com>
> ---
>  ports/linux/guts/fcntl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ports/linux/guts/fcntl.c b/ports/linux/guts/fcntl.c
> index 639fd24..d278a8c 100644
> --- a/ports/linux/guts/fcntl.c
> +++ b/ports/linux/guts/fcntl.c
> @@ -8,6 +8,10 @@
>   */
>         long arg;
>         int save_errno;
> +       /* some bits can be ORed into the cmd should be explicitly ignored
> +        * see fcntl documentation on F_SETFL */
> +       int o_mode_mask = (O_RDONLY | O_WRONLY | O_RDWR) |
> +               (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>
>         /* we don't know whether we need lock or arg; grab both, which
>          * should be safe enough on Linuxy systems. */
> @@ -15,7 +19,7 @@
>         arg = va_arg(ap, long);
>         va_end(ap);
>
> -       switch (cmd) {
> +       switch (cmd & ~(o_mode_mask)) {
>         case F_DUPFD:
>  #ifdef F_DUPFD_CLOEXEC
>         case F_DUPFD_CLOEXEC:
> --
> 2.7.4
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>

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

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Will Page]

Thanks, Ross.

I had contacted the maintainer off list to ask where I should submit my patch.  I may have misunderstood his reply.

I'll go ahead and resend this patch to the yocto list.

________________________________________________________

Will Page
Senior Software Engineer
National instruments

O +1.512.683.6956
Will.Page@ni.com


________________________________________
From: Burton, Ross <ross.burton@intel.com>
Sent: Friday, September 15, 2017 3:48 PM
To: Will Page
Cc: OE-core; Seebs
Subject: Re: [OE-core] [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

Hi Will,

This should go to the yocto@yoctoproject.org<mailto:yocto@yoctoproject.org> list really.  I've CC'd the maintainer to be sure he spots it.

Ross

On 15 September 2017 at 23:27, Will Page <Will.Page@ni.com<mailto:Will.Page@ni.com>> wrote:
The fcntl guts switch on "cmd" parameter to identify the fcntl
command being issued, but isn't aware of the file creation flags that
can be ORed in.

This change masks out the flags from the command only for the switch
statement so that it can correctly determine whether it needs to pass
"lock" or "arg".  When real_fcntl() is called, the original value is
still passed on. This corrects an issue observed using pseudo on modern
linux desktops resulting in "pseudo: unknown fcntl argument 1030,
assuming long argument." diagnostics in the output.  Decimal 1030 is
0x0406, which translates to O_NOCTTY | F_SETLK, and should call
"rc = real_fcntl(fd, cmd, lock);", but instead falls through to the
default case instead calling "rc = real_fcntl(fd, cmd, arg);".

Tested the change using perftest - without this patch, the issue is
trivially reproducible on my Ubuntu Xenial system, and after patching it
emitted no "unknown fcntl argument" diagnostics.

Signed-off-by: Will Page <Will.Page@ni.com<mailto:Will.Page@ni.com>>
---
 ports/linux/guts/fcntl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ports/linux/guts/fcntl.c b/ports/linux/guts/fcntl.c
index 639fd24..d278a8c 100644
--- a/ports/linux/guts/fcntl.c
+++ b/ports/linux/guts/fcntl.c
@@ -8,6 +8,10 @@
  */
        long arg;
        int save_errno;
+       /* some bits can be ORed into the cmd should be explicitly ignored
+        * see fcntl documentation on F_SETFL */
+       int o_mode_mask = (O_RDONLY | O_WRONLY | O_RDWR) |
+               (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);

        /* we don't know whether we need lock or arg; grab both, which
         * should be safe enough on Linuxy systems. */
@@ -15,7 +19,7 @@
        arg = va_arg(ap, long);
        va_end(ap);

-       switch (cmd) {
+       switch (cmd & ~(o_mode_mask)) {
        case F_DUPFD:
 #ifdef F_DUPFD_CLOEXEC
        case F_DUPFD_CLOEXEC:
--
2.7.4

--
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org<mailto:Openembedded-core@lists.openembedded.org>
http://lists.openembedded.org/mailman/listinfo/openembedded-core<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.openembedded.org_mailman_listinfo_openembedded-2Dcore&d=DwMFaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=-PS0OYZ7YP1crHfex64Ojw&m=GwerYyJqZysRsEI2Ua5_9TJVT-a8KZxs2fec7a3M6ig&s=_U2-_Ub2i1xoGVV3H1m1I2IuNiovYWvZUapKZG7swPY&e=>

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Seebs]

On Sat, 16 Sep 2017 02:22:01 +0000
Will Page <will.page@ni.com> wrote:

> Thanks, Ross.
> 
> I had contacted the maintainer off list to ask where I should submit
> my patch.  I may have misunderstood his reply.

No, I'm just confused, because I'm not really otherwise active in
yocto/oe-core these days, I just still know my way around pseudo. (And
I know I'm several patches behind on things, but I'm really hoping to
get some free time in a couple of weeks.)

-s

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Seebs]

On Fri, 15 Sep 2017 15:27:00 -0700
Will Page <Will.Page@ni.com> wrote:

> The fcntl guts switch on "cmd" parameter to identify the fcntl
> command being issued, but isn't aware of the file creation flags that
> can be ORed in.

This is true. I was, in fact, not aware of those flags. Looks
reasonable.

-s

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Richard Purdie]

On Fri, 2017-09-15 at 18:10 -0500, Seebs wrote:
> On Fri, 15 Sep 2017 15:27:00 -0700
> Will Page <Will.Page@ni.com> wrote:
> 
> > 
> > The fcntl guts switch on "cmd" parameter to identify the fcntl
> > command being issued, but isn't aware of the file creation flags
> > that
> > can be ORed in.
> This is true. I was, in fact, not aware of those flags. Looks
> reasonable.

I tried adding this to a test build along with AlexK's epoll patch. It
resulted in:

https://autobuilder.yocto.io/builders/nightly-world/builds/475/steps/BuildImages/logs/stdio

Any ideas why?

Cheers,

Richard

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Will Page]

On Mon, Sep 18, 2017 at 02:24:26PM +0100, Richard Purdie wrote:
> On Fri, 2017-09-15 at 18:10 -0500, Seebs wrote:
> > On Fri, 15 Sep 2017 15:27:00 -0700
> > Will Page <Will.Page@ni.com> wrote:
> > 
> > > 
> > > The fcntl guts switch on "cmd" parameter to identify the fcntl
> > > command being issued, but isn't aware of the file creation flags
> > > that
> > > can be ORed in.
> > This is true. I was, in fact, not aware of those flags. Looks
> > reasonable.
> 
> I tried adding this to a test build along with AlexK's epoll patch. It
> resulted in:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__autobuilder.yocto.io_builders_nightly-2Dworld_builds_475_steps_BuildImages_logs_stdio&d=DwICaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=-PS0OYZ7YP1crHfex64Ojw&m=xLMg1sihnYcY8mm3vbgoq_awtDy2SclWmfP3SxUNhq0&s=qFv8uK99a-cleN6VLJ7XYfBiLHrcTKtPo63E8FmGuX0&e= 
> 
> Any ideas why?
> 
> Cheers,
> 
> Richard

The problem is mine.  I sent you the wrong revision of the patch.
+	int o_mode_mask = (O_RDONLY | O_WRONLY | O_RDWR) |
+		(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);

In the first rev of my patch (which I accidentally sent in place of my
intended changes), I included the O_ACCMODE flags because it looked like
the documentation said they should also be ignored, but that's not
actually true.  The documentation says they will be ignored in the
"arg" parameter for F_SETFL commands, not in the "cmd" parameter.

After additional research, I found that my problem case actually relates
to some "new", linux-specific extensions to fcntl:

#ifdef __USE_GNU
# define F_SETLEASE        1024        /* Set a lease.  */
# define F_GETLEASE        1025        /* Enquire what lease is active.
  */
# define F_NOTIFY        1026        /* Request notifications on a
directory.  */
# define F_SETPIPE_SZ        1031        /* Set pipe page size array.
  */
# define F_GETPIPE_SZ        1032        /* Set pipe page size array.
  */
#endif
#ifdef __USE_XOPEN2K8
# define F_DUPFD_CLOEXEC 1030        /* Duplicate file descriptor with
                                   close-on-exit set.  */
#endif

https://code.woboq.org/userspace/glibc/sysdeps/unix/sysv/linux/bits/fcntl-linux.h.html

Consequently, a few new cases are needed in the switch statement, with
similar ifdef guards to the F_*LK64 cases.

I'll submit V2 of my patch shortly.

Re: [pseudo][PATCH] Fix to fcntl guts to ignore flags that can be ORed into cmd

[Will Page]

On Mon, Sep 18, 2017 at 02:24:26PM +0100, Richard Purdie wrote:
> On Fri, 2017-09-15 at 18:10 -0500, Seebs wrote:
> > On Fri, 15 Sep 2017 15:27:00 -0700
> > Will Page <Will.Page@ni.com> wrote:
> > 
> > > 
> > > The fcntl guts switch on "cmd" parameter to identify the fcntl
> > > command being issued, but isn't aware of the file creation flags
> > > that
> > > can be ORed in.
> > This is true. I was, in fact, not aware of those flags. Looks
> > reasonable.
> 
> I tried adding this to a test build along with AlexK's epoll patch. It
> resulted in:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__autobuilder.yocto.io_builders_nightly-2Dworld_builds_475_steps_BuildImages_logs_stdio&d=DwICaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=-PS0OYZ7YP1crHfex64Ojw&m=xLMg1sihnYcY8mm3vbgoq_awtDy2SclWmfP3SxUNhq0&s=qFv8uK99a-cleN6VLJ7XYfBiLHrcTKtPo63E8FmGuX0&e= 
> 
> Any ideas why?
> 
> Cheers,
> 
> Richard

I'm sorry - I was looking over v2 of my patch, and something just wasn't
adding up.

pseudo already supports F_DUPFD_CLOEXEC (which turned out
to be the problematic command for me).  The problem was on my end - I
had two copies of the binary: one that was built on a system where
F_DUPFD_CLOEXEC was defined, and one built on a system where it wasn't.

I repeat, the problem was entirely on my end.  I respectfully retract my
patch, prior statements, and I thank you for your time.

Will Page