Fix to fcntl guts to ignore flags that can be ORed into cmd

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.

---
 ports/linux/guts/fcntl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 639fd24..d278a8c 100644
@@ -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

== 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 ([email protected]) 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

Hi Will,

This should go to the [email protected] list really. I've CC'd the maintainer to be sure he spots it.

Ross

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

________________________________________
From: Burton, Ross <[email protected]>
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 [email protected]<mailto:[email protected]> list really. I've CC'd the maintainer to be sure he spots it.

Ross

On 15 September 2017 at 23:27, Will Page <[email protected]<mailto:[email protected]>> 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.

---
 ports/linux/guts/fcntl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 639fd24..d278a8c 100644
@@ -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 [email protected]<mailto:[email protected]>
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=>

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

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

-s

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

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.

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