Check fixed args number for variadic function #4122

tiif · 2025-01-04T18:07:35Z

This PR added

check_shim_variadic to be used by variadic function shims
check_min_vararg_count to retrieve varargs array from slice
check_fixed_args_count to check if we got expected number of fixed args

cc #4013

oli-obk

Please squash

src/shims/unix/foreign_items.rs

src/helpers.rs

tiif · 2025-01-07T11:22:58Z

@rustbot author

tiif · 2025-01-14T05:31:54Z

So now there is this weird function definition in this PR

    fn open(
        &mut self,
        args: &[OpTy<'tcx>],
        fixed: &[OpTy<'tcx>; 2],
        _var: &[OpTy<'tcx>],
    ) -> InterpResult<'tcx, Scalar> {

where args is the full function argument, fixed is the fixed args, and _var is var args.

Since we already split fixed and var args through check_shim_variadic before we call this.open, I have tried to not pass args into open, but the problem is if I avoid using args and do this instead

            let [mode] = check_min_arg_count("open(pathname, O_CREAT, ...)", var)?

It will lead to diagnostic like this:

11 |     let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) }; 
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of arguments for `open(pathname, O_CREAT, ...)`: got 0, expected at least 1

Or maybe we can just ignore the fixed args and var args returned by check_shim_variadic?

Co-authored-by: Ralf Jung <[email protected]>

RalfJung · 2025-01-14T07:44:40Z

It will lead to diagnostic like this:

I would suggest to rename check_min_arg_count to check_min_vararg_count and make the error say something like "not enough variadic arguments for ...".

src/shims/unix/linux_like/syscall.rs

tiif · 2025-01-17T16:15:11Z

I didn't manage completely replace check_min_arg_count with check_min_vararg_count. There is only left with one check_min_arg_count, which is

miri/src/shims/backtrace.rs

Lines 32 to 42 in c2a3761

    
           fn handle_miri_get_backtrace( 
        
               &mut self, 
        
               abi: &FnAbi<'tcx, Ty<'tcx>>, 
        
               link_name: Symbol, 
        
               args: &[OpTy<'tcx>], 
        
               dest: &MPlaceTy<'tcx>, 
        
           ) -> InterpResult<'tcx> { 
        
               let this = self.eval_context_mut(); 
        
               let tcx = this.tcx; 
        
               let [flags] = check_min_arg_count("miri_get_backtrace", args)?;

I think as a miri specific extern function, the argument here shouldn't be classified as a vararg?

tiif · 2025-01-17T16:26:31Z

src/shims/unix/foreign_items.rs

@@ -207,8 +207,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
            "fcntl" => {
                // `fcntl` is variadic. The argument count is checked based on the first argument


This comment looks weird, the argument count should be checked based on the second argument cmd instead of the first? (source)

Probably a copy-paste mistake.

tiif · 2025-01-17T16:51:57Z

@rustbot ready

RalfJung · 2025-01-19T17:22:05Z

I think as a miri specific extern function, the argument here shouldn't be classified as a vararg?

Oh dang.

We have a comment in the docs for this function saying "The flags argument must be 1."
So could you try making a PR that forces the function to have 2 arguments and flags == 1? We recently did the same with miri_resolve_frame, removing support for the old "version 0 protocol"; if we do the same here then we can get rid of this use of check_min_arg_count hopefully. :)

RalfJung

I left some comments on the first place where you used the new infra; they apply everywhere in the PR.

RalfJung · 2025-01-19T17:22:40Z

src/helpers.rs

@@ -928,6 +928,30 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
        interp_ok(())
    }

+    /// Check the number of fixed args.


Suggested change

/// Check the number of fixed args.

/// Check the number of fixed args of a vararg function.

This should probably error if the function is not a vararg fn?

Please also put "vararg" in the name then, e.g. check_vargarg_fixed_arg_count.

(We used arg_count before so please don't use args_count here now.)

RalfJung · 2025-01-19T17:23:58Z

src/helpers.rs

+        &'a [OpTy<'tcx>; N]: TryFrom<&'a [OpTy<'tcx>]>,
+    {
+        self.check_abi_and_shim_symbol_clash(abi, exp_abi, link_name)?;
+        self.check_fixed_args_count(shim_name, abi, args)


Why does this need self when check_arg_count does not?

RalfJung · 2025-01-19T17:25:32Z

src/helpers.rs

@@ -1183,7 +1224,7 @@ where
    throw_ub_format!("incorrect number of arguments: got {}, expected {}", args.len(), N)
 }

-/// Check that the number of args is at least the minumim what we expect.
+/// Check that the number of args is at least the minimum what we expect.


Suggested change

/// Check that the number of args is at least the minimum what we expect.

/// Check that the number of args is at least the minimum what we expect.

/// FIXME: Remove this function, use varargs and `check_min_vararg_count` instead.

RalfJung · 2025-01-19T17:26:00Z

src/shims/unix/android/thread.rs

-    // We do not use `check_shim` here because `prctl` is variadic. The argument
-    // count is checked bellow.
-    ecx.check_abi_and_shim_symbol_clash(abi, Conv::C, link_name)?;
+    // `prctl` is variadic. The argument count is checked below.


Suggested change

// `prctl` is variadic. The argument count is checked below.

Not sure which value this comment still adds.

RalfJung · 2025-01-19T17:26:45Z

src/shims/unix/android/thread.rs


    // FIXME: Use constants once https://github.com/rust-lang/libc/pull/3941 backported to the 0.2 branch.
    let pr_set_name = 15;
    let pr_get_name = 16;

-    let [op] = check_min_arg_count("prctl", args)?;
+    let [op] = fixed_args;


Suggested change

let [op] = fixed_args;

RalfJung · 2025-01-19T17:27:12Z

src/shims/unix/android/thread.rs

+    let (fixed_args, varargs) =
+        ecx.check_shim_variadic::<1>(abi, Conv::C, link_name, "prctl", args)?;


Suggested change

let (fixed_args, varargs) =

ecx.check_shim_variadic::<1>(abi, Conv::C, link_name, "prctl", args)?;

let ([op], varargs) =

ecx.check_shim_variadic(abi, Conv::C, link_name, "prctl", args)?;

Same everywhere else -- you should never use turbofish here.

RalfJung · 2025-01-19T17:27:51Z

src/shims/unix/fd.rs

-    fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
+    fn fcntl(
+        &mut self,
+        fixed_args: &[OpTy<'tcx>; 2],


Instead of passing a 2-element array, just pass two arguments.

RalfJung · 2025-01-19T17:28:39Z

@rustbot author

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Jan 4, 2025

oli-obk approved these changes Jan 7, 2025

View reviewed changes

RalfJung reviewed Jan 7, 2025

View reviewed changes

src/shims/unix/foreign_items.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jan 7, 2025

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jan 7, 2025

This comment has been minimized.

Sign in to view

tiif and others added 11 commits January 14, 2025 13:34

Add fixed args count check for open

bfc626a

Add test for fixed args count

b10bb99

Add check for other variadic functions

7f915d2

Better format

a64c116

Change diagnostic wording

b6acfa9

Co-authored-by: Ralf Jung <[email protected]>

Add check_shim_variadic

eb844f3

Change a few fixed arg handling

26f231b

Change fixed arg handling

1e956b1

Bless the test

7b1c08a

Fix var arg handling

317d68d

fmt

b95b2c6

tiif force-pushed the fnabi branch from 909364c to b95b2c6 Compare January 14, 2025 05:42

oli-obk reviewed Jan 15, 2025

View reviewed changes

src/shims/unix/linux_like/syscall.rs Outdated Show resolved Hide resolved

tiif added 2 commits January 18, 2025 00:10

Add check_min_vararg count and use it

476fd11

Bless test

9aa7f98

Remove unnecessary const argument

b7039cd

tiif commented Jan 17, 2025

View reviewed changes

tiif added 2 commits January 18, 2025 00:33

Change function arguments name

df32a54

More rename

c8510d4

tiif added 2 commits January 18, 2025 00:43

fmt

65963d6

Tweak some comments

15109d2

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jan 17, 2025

RalfJung reviewed Jan 19, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jan 19, 2025

tiif mentioned this pull request Jan 21, 2025

Implement Display for rustc_target::callconv::Conv rust-lang/rust#135808

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check fixed args number for variadic function #4122

Check fixed args number for variadic function #4122

tiif commented Jan 4, 2025 •

edited

Loading

oli-obk left a comment

tiif commented Jan 7, 2025

This comment has been minimized.

tiif commented Jan 14, 2025 •

edited

Loading

RalfJung commented Jan 14, 2025

tiif commented Jan 17, 2025 •

edited

Loading

tiif Jan 17, 2025 •

edited

Loading

RalfJung Jan 19, 2025

tiif commented Jan 17, 2025

RalfJung commented Jan 19, 2025

RalfJung left a comment

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung Jan 19, 2025

RalfJung commented Jan 19, 2025

		@@ -207,8 +207,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
		"fcntl" => {
		// `fcntl` is variadic. The argument count is checked based on the first argument

	/// Check the number of fixed args.
	/// Check the number of fixed args of a vararg function.

	/// Check that the number of args is at least the minimum what we expect.
	/// Check that the number of args is at least the minimum what we expect.
	/// FIXME: Remove this function, use varargs and `check_min_vararg_count` instead.

		let (fixed_args, varargs) =
		ecx.check_shim_variadic::<1>(abi, Conv::C, link_name, "prctl", args)?;

Check fixed args number for variadic function #4122

Are you sure you want to change the base?

Check fixed args number for variadic function #4122

Conversation

tiif commented Jan 4, 2025 • edited Loading

oli-obk left a comment

Choose a reason for hiding this comment

tiif commented Jan 7, 2025

This comment has been minimized.

tiif commented Jan 14, 2025 • edited Loading

RalfJung commented Jan 14, 2025

tiif commented Jan 17, 2025 • edited Loading

tiif Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiif commented Jan 17, 2025

RalfJung commented Jan 19, 2025

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jan 19, 2025

tiif commented Jan 4, 2025 •

edited

Loading

tiif commented Jan 14, 2025 •

edited

Loading

tiif commented Jan 17, 2025 •

edited

Loading

tiif Jan 17, 2025 •

edited

Loading