17 ms·

From [1]: > You are not "introducing" a new macro for this, you are modifying the existing one such that all users of it now have the select_nospec() call in i

From [1]: > You are not "introducing" a new macro for this, you are modifying the existing one such that all users of it now have the select_nospec() call in it. > Is that intentional? This is going to hit a _lot_ of existing entries that probably do not need it at all. > Why not just create list_for_each_entry_nospec()? Let's ignore whether this patch is needed or works for now - I don't feel competent to comment on that. But this is such a bad suggestion. Instead of fixing the default to be safe, and possibly having a _unsafe_but_fast() variant for the places where it makes sense they want to keep the broken version and require user to explicitly opt in into safety. Same as the infamous PHP mysql_real_escape_string (don't make the mistake of using mysql_escape_string!) [2][3] or whole host of C stdlib footguns like strcpy/strncpy [4]. The default, easy, obvious option should be safe. The unsafe but faster option should be hard to use by accident and obviously marked as such from the name. [1] https://lwn.net/ml/linux-kernel/Yg6iCS0XZB6EtMP7@kroah.com/ https://lwn.net/ml/linux-kernel/Yg6iCS0XZB6EtMP7@kroah.com/ [2] https://www.php.net/manual/en/function.mysql-escape-string.php https://www.php.net/manual/en/function.mysql-escape-string.p... [3] https://www.php.net/manual/en/function.mysql-real-escape-string.php https://www.php.net/manual/en/function.mysql-real-escape-str... [4] https://en.cppreference.com/w/cpp/header/cstring https://en.cppreference.com/w/cpp/header/cstring

masklinn 5y ago
> Same as the infamous PHP mysql_real_escape_string (don't make the mistake of using mysql_escape_string!) [2][3] TBF that is really a straight bridge to the mysql C API, which is why it’s also in mysqli. MySQL actually has a third one called mysql_real_escape_string_quote, because if the sql mode NO_BACKSLASH_ESCAPES is set the escaping function needs to know what context it’s used in, and thus what quote to double up, and mysql_real_escape_string will fail.
- account42 5y ago
  And of course, the real answer to wether you're supposed to use mysql_escape_string, mysql_real_escape_string or mysql_real_escape_string_quote is "why the fuck are you pasting queries together using string processing".
deleted 5y ago
[deleted]
klodolph 5y ago
This is just how you work with large code bases. Updating 15,000 call sites is far from trivial. These changes happen in multiple phases, and in each phase, you still want to have a working kernel. The unsafe version can be deprecated and eventually removed, but that is down the road.
- xiphias2 5y ago
  I’m not sure how it’s done with the Linux Kernel (and git definately makes it hard), but at Google (using Perfoce model) there are tools for mass renaming that try to guarantee that there are no regressions. Of course in Linux the amount of C macros make it much harder to work on the syntax trees, but comparision after the preprocessor step should be possible.
  - klodolph 5y ago
    IIRC Google works the same way. The automated refactoring tools (Rosie?) may make it go faster, but you generally don't fix up 15,000 call sites in a single change... you break it up into smaller batches spread across the tree.
    - acidbaseextract 5y ago
      It's been a long time since I was there, but I thought there were plenty of 15,000 call site refactorings done in a single final CL. Not that the Linux kernel should do the same!
      - skybrian 5y ago
        When I was there (several years ago) it was rare to do that across many projects. You don't want to have to roll back everyone due to a problem that affects one project. Also, there's a risk that changes that are happening in the meantime might mean the patch doesn't apply.
        Filligree 5y ago
toast0 5y ago
> Same as the infamous PHP mysql_real_escape_string (don't make the mistake of using mysql_escape_string!) [2][3] or whole host of C stdlib footguns like strcpy/strncpy In all of those, the enhanced function takes more paramaters. Switching the existing name to require a new parameter will break existing code, and people will not want to update. Making the new parameter optional is possible, but IMHO is messier than a new function that has required parameters and then deprecating the old function (and eventually removing it).
marcosdumay 5y ago
That's the deal with backwards compatibility. Even third parties may depend on that name, it's not safe to go and change it. On userspace that's the time when you deprecate an entire module and switch everything into a new name. The kernel does that once in a long while, but it's a bit harder for them.
- marcan_42 5y ago
  The kernel doesn't care about third parties though, that's why the kernel API/ABI is not stable. This is one big advantage to doing it that way: sure, you need to have a consistent migration plan in flight to make sure you don't break mainline in the process, but you can immediately remove the old way without having to care about third parties, once you're done.
stephenr 5y ago
I get the point you’re trying to make, but the blame doesn’t really lay with php. Like most things introduced in early php, the mysql extension was just wrapping c functions. The concept of “real” escape comes from mysqls’ C API: https://dev.mysql.com/doc/c-api/5.6/en/mysql-real-escape-string.html https://dev.mysql.com/doc/c-api/5.6/en/mysql-real-escape-str...
midjji 5y ago
You are making the rather optimistic assumption that here is a single definition of the macro. There are likely dozens, most of which share a name and randomly overwrite each other, others share the name but are subtly different. Macros are a truly terrible way to metaprogram. In my experience, if you spot one in a code base and can remove it, you have removed a bug you never knew you had, you spot one and change it, you have broken more things than you will ever know.

masklinn 5y ago

> Same as the infamous PHP mysql_real_escape_string (don't make the mistake of using mysql_escape_string!) [2][3] TBF that is really a straight bridge to the mysql C API, which is why it’s also in mysqli. MySQL actually has a third one called mysql_real_escape_string_quote, because if the sql mode NO_BACKSLASH_ESCAPES is set the escaping function needs to know what context it’s used in, and thus what quote to double up, and mysql_real_escape_string will fail.

account42 5y ago
And of course, the real answer to wether you're supposed to use mysql_escape_string, mysql_real_escape_string or mysql_real_escape_string_quote is "why the fuck are you pasting queries together using string processing".

deleted 5y ago

[deleted]

klodolph 5y ago

This is just how you work with large code bases. Updating 15,000 call sites is far from trivial. These changes happen in multiple phases, and in each phase, you still want to have a working kernel. The unsafe version can be deprecated and eventually removed, but that is down the road.

xiphias2 5y ago
I’m not sure how it’s done with the Linux Kernel (and git definately makes it hard), but at Google (using Perfoce model) there are tools for mass renaming that try to guarantee that there are no regressions. Of course in Linux the amount of C macros make it much harder to work on the syntax trees, but comparision after the preprocessor step should be possible.
- klodolph 5y ago
  IIRC Google works the same way. The automated refactoring tools (Rosie?) may make it go faster, but you generally don't fix up 15,000 call sites in a single change... you break it up into smaller batches spread across the tree.
  - acidbaseextract 5y ago
    It's been a long time since I was there, but I thought there were plenty of 15,000 call site refactorings done in a single final CL. Not that the Linux kernel should do the same!
    - skybrian 5y ago
      When I was there (several years ago) it was rare to do that across many projects. You don't want to have to roll back everyone due to a problem that affects one project. Also, there's a risk that changes that are happening in the meantime might mean the patch doesn't apply.
      - Filligree 5y ago

toast0 5y ago

> Same as the infamous PHP mysql_real_escape_string (don't make the mistake of using mysql_escape_string!) [2][3] or whole host of C stdlib footguns like strcpy/strncpy In all of those, the enhanced function takes more paramaters. Switching the existing name to require a new parameter will break existing code, and people will not want to update. Making the new parameter optional is possible, but IMHO is messier than a new function that has required parameters and then deprecating the old function (and eventually removing it).

marcosdumay 5y ago

That's the deal with backwards compatibility. Even third parties may depend on that name, it's not safe to go and change it. On userspace that's the time when you deprecate an entire module and switch everything into a new name. The kernel does that once in a long while, but it's a bit harder for them.

marcan_42 5y ago
The kernel doesn't care about third parties though, that's why the kernel API/ABI is not stable. This is one big advantage to doing it that way: sure, you need to have a consistent migration plan in flight to make sure you don't break mainline in the process, but you can immediately remove the old way without having to care about third parties, once you're done.

stephenr 5y ago

I get the point you’re trying to make, but the blame doesn’t really lay with php. Like most things introduced in early php, the mysql extension was just wrapping c functions. The concept of “real” escape comes from mysqls’ C API: https://dev.mysql.com/doc/c-api/5.6/en/mysql-real-escape-string.html https://dev.mysql.com/doc/c-api/5.6/en/mysql-real-escape-str...

midjji 5y ago

You are making the rather optimistic assumption that here is a single definition of the macro. There are likely dozens, most of which share a name and randomly overwrite each other, others share the name but are subtly different. Macros are a truly terrible way to metaprogram. In my experience, if you spot one in a code base and can remove it, you have removed a bug you never knew you had, you spot one and change it, you have broken more things than you will ever know.