7 ms·

Was just looking at commits and came across a commit and its revert original commit: https://github.com/RsyncProject/rsync/commit/d046525de39315d625ffaef4fdd6e

Was just looking at commits and came across a commit and its revert original commit: https://github.com/RsyncProject/rsync/commit/d046525de39315d625ffaef4fdd6e7cf12148016 https://github.com/RsyncProject/rsync/commit/d046525de39315d... ``` - if (!ptr) - ptr = malloc(num * size); - else if (ptr == do_calloc) + if (!ptr || ptr == do_calloc) ptr = calloc(num, size); ``` Written with claude. This is a good example of what slips through LLM attention. It forces all allocations to be calloc as if it is a strict upgrade. For large and recursive allocations, this becomes a significant cost. reverted in https://github.com/RsyncProject/rsync/commit/7db73ad9a1b8721f14a43219d73127b23b86fe00 https://github.com/RsyncProject/rsync/commit/7db73ad9a1b8721... if you read the description of revert half carefully, it's easy to tell that even that was written by an LLM . I can understand the sentiment of whoever posted the original thread.

wolletd 4mo ago
Also the amount of commits is suspicious. In the last two months, rsync had about as much commits as in the last two years before that. Most of them written with claude. And then stuff like this is in there. That's exactly what I'd expect when someone is excited about AI usage and becomes... well, sloppy.
- logicprog 4mo ago
  Tridge already explains this: "Like many developers of open source packages I’ve been hit by a flood of security reports lately in my role as the rsync maintainer. Many of those reports are AI generated (not all though, there are some notable ones with very careful and high quality manual analysis). As this flood started to get more intense I realised I needed to raise the defences on rsync a lot — we needed much more thorough test suites, code coverage analysis, CI testing on a lot more platforms, deliberate and thorough scanning for possible security issues (so I find at least some of them before other people!) and the addition of a whole lot of defence-in-depth hardening techniques. This is all a huge amount of work. " https://medium.com/@tridge60/rsync-and-outrage-d9849599e5a0 https://medium.com/@tridge60/rsync-and-outrage-d9849599e5a0
  - rendaw 4mo ago
    Is using calloc for everything fixing a security issue or hardening it?
    - ekidd 4mo ago
      Calloc is generally hardening, because it zeros out any stale memory contents left over from previous uses of the memory. You can avoid this overhead if you use a language that forbids reading from uninitialized memory, but C is not that language.
      - kelnos 4mo ago
        Uninitialized memory is not a problem (the OS is never going to give a program memory that has data in it from another program). The problem is memory that you allocated in the past, have freed, but hasn't been returned to the OS[0]. It might have key material or other sensitive data in it[1]. Or it might just have random garbage in it that could be misinterpreted by the code that's about to use it, if it hasn't been initialized to a known state. For some uses, you do genuinely need (specifically) zeroed-out memory before you start to use it, and that's where calloc() is truly useful. But that need not have anything to do with security. [0] The allocator will often hold onto memory that has been freed in order to quickly service future requests for new allocations, without needing a context switch into kernel space. [1] Granted, the correct way to handle that is to zero it out before freeing it, in a way that the compiler won't optimize out.
        ekidd 4mo ago
        > The problem is memory that you allocated in the past, have freed, but hasn't been returned to the OS[0]. There are at least two different ways in which memory might be semantically "uninitialized": 1. The memory was provided by the OS. On modern desktop and mobile OSes, this memory will normally be zeroed automatically. 2. The memory was provided by the language's allocator. This may contain a mix of data used by previous allocations and memory that has never been touched (perhaps because previous allocations reserved it as end-of-array "capacity" that never got used). From the perspective of a language like Rust, this memory is considered uninitialized, and safe code should never be able to read it without first setting it. In ancient C code, it makes a fair bit of sense to preemptively calloc everything. Or better, to wrap the allocator with one that zeroes on free. Though even there, you need to be careful not to expose recycled heap block headers in the middle of newly allocated objects. My opinion for the last 30+ years has been that C is unfit for purpose, and that using it almost inevitably introduces large numbers of dire security holes. But until the last 10-15 years, there hasn't been any seriously viable alternatives.
- gravypod 4mo ago
  > Also the amount of commits is suspicious. In the last two months, rsync had about as much commits as in the last two years before that. I wonder if the data looks worse or better when not doing per-10commit and instead do per-commit.
- echelon 4mo ago
  Seems like someone could use Claude to port rsync to Rust and the whole enterprise would be safer from things like this. Start with unsafe then gradually convert into idiomatic Rust.
  - yubblegum 4mo ago
    Your let's redo this in Rust made me wonder if generative AI will also be susceptible to software fads. One LLM writes a few blog posts extoling a new framework/lanaguge. Other agentics read these and get 'influenced'. Then they start clamoring for 'lets redo this in X!'. Can't wait to see it. /g
    - whattheheckheck 4mo ago
      We will need rigorous agnostic statistical experiments to know what stuff is better
    - globalnode 4mo ago
      its bad enough when humans do it
    - bryanrasmussen 4mo ago
      Prompt: automate writing commits to increase safety in these software projects so that my profile increases and I can snag a high-paying Rust job. LLM: this commit changes whole codebase to Rust!
    - kajaktum 4mo ago
      You can get 80% there with rust which is what is impressive. Then you have a reference implementation that you can always check against. If a Rust library have 0 unsafe, i dont care if it is written by a dog, it still have 0 UB.
      - klabb3 4mo ago
        UB is especially bad but also not as big as all other concerns combined. Two of the most reliable software ever to exist, curl and SQLite, are C/C++. There are also cases in system programming, drivers etc where the unsafe is necessary and then your code is only as good as the boundary, and lots of bugs can seep in. Another issue with Rust is ecosystem - the dependency trees required to do fairly basic things are often deep and vast, meaning other risks. That said if something like rsync was written today, I still think Rust may be a better choice. Mainly because a 95 percentile skilled Rust programmer is less dangerous than for C. The people that are skilled enough to be trusted with C are few and diminishing every year.
- lokar 4mo ago
  I would expect a 10x change rate, even carried out by clones of the existing maintainers to result in more bugs.
- whateveracct 4mo ago
  mythical man month only gets more prescient as time passes
scottlamb 4mo ago
> This is a good example of what slips through LLM attention. It forces all allocations to be calloc as if it is a strict upgrade. I wouldn't assume Claude made that decision; it's not as if that was some incidental thing that it snuck into a large commit. The commit message starts with "zero all new memory from allocations", and that's exactly what the commit does. What do you imagine the prompt was? It seems totally plausible to me that a human initially thought this was an improvement, then rethought after discovering the RSS regression. And it's not a law of nature anyway that this change has to increase RSS; calloc could special-case the case in which memory was freshly returned from the OS, knowing fresh memory mappings are zeroed anyway. I blame AI for these regressions mostly in the sense that it caused a flurry of vulnerability reports. Those led to a flurry of quick fixes. Sometimes quick fixes cause other problems.
- delusional 4mo ago
  You don't really have to guess. The guy told us the AI didn't suggest this specific change: > The change to zero memory was my idea and my change. It was a reaction to a security report I got which caused use of an element past the end of an array. By zeroing the allocation I could ensure that misuse of that memory if a similar bug came up in the future could only cause a null ptr deref, which is better than the chance of a valid pointer. It got a claude co-authored tag on it as I got it to do some tidy ups of a series of commits, and that is just what it does when it makes any modification. It doesn't mean the change was written by claude. It was written by me. https://github.com/RsyncProject/rsync/issues/959#issuecomment-4625643726 https://github.com/RsyncProject/rsync/issues/959#issuecommen...
  - jagged-chisel 4mo ago
    > … By zeroing the allocation … How does that prevent reading past the end of the buffer? Or change how bytes outside the buffer are used? Are these arrays of pointers so that the “null ptr deref” comment makes sense? Or am I the bozo and don’t know what’s happening here?
    - kccqzy 4mo ago
tom_ 4mo ago
AI multiplied by Linux overcommit. What times we live in! (My own view: 10.8 GB is nothing these days. Your sprintf buffers are probably larger than that. (And if they aren't: they should be. That, or you should start using snprintf...))
- baq 4mo ago
  sprintf() should be a longer way to write abort(), change my mind
  - bruce343434 4mo ago
    I'll change your mind: If you pass NULL as the destination pointer, it doesn't write any string. If you combine this with %n at the end of the format string, you can get the exact length that the output string would be. Then you allocate that, then you print again, into the actual destination buffer this time.
    - baq 4mo ago
      If anything I just got entrenched in my opinion. The second best option, maybe, would be to not accept any destination pointers and have the default and only possible behavior just like what you describe.
      - bruce343434 4mo ago
        I agree. I tend to use the gnu "asprintf" which simply returns a properly allocated char buffer with the formatted string in it. And on platforms that don't feature asprintf (windows) you can build your own using sprintf!
CaliforniaKarl 4mo ago
> Written with claude. No. The reversion commit references https://github.com/RsyncProject/rsync/issues/959 https://github.com/RsyncProject/rsync/issues/959. In that GitHub issue is this comment: > The change to zero memory was my idea and my change. It was a reaction to a security report I got which caused use of an element past the end of an array. By zeroing the allocation I could ensure that misuse of that memory if a similar bug came up in the future could only cause a null ptr deref, which is better than the chance of a valid pointer. > It got a claude co-authored tag on it as I got it to do some tidy ups of a series of commits, and that is just what it does when it makes any modification. It doesn't mean the change was written by claude. It was written by me.
alfiedotwtf 4mo ago
AI is fine, and in fact fun to use... committing AI written code without understanding Every. Single. Line. Of. Changes is on the committer. You can't LGFM for vibe code ffs
KaiShips 4mo ago
[flagged]

wolletd 4mo ago

Also the amount of commits is suspicious. In the last two months, rsync had about as much commits as in the last two years before that. Most of them written with claude. And then stuff like this is in there. That's exactly what I'd expect when someone is excited about AI usage and becomes... well, sloppy.

logicprog 4mo ago
Tridge already explains this: "Like many developers of open source packages I’ve been hit by a flood of security reports lately in my role as the rsync maintainer. Many of those reports are AI generated (not all though, there are some notable ones with very careful and high quality manual analysis). As this flood started to get more intense I realised I needed to raise the defences on rsync a lot — we needed much more thorough test suites, code coverage analysis, CI testing on a lot more platforms, deliberate and thorough scanning for possible security issues (so I find at least some of them before other people!) and the addition of a whole lot of defence-in-depth hardening techniques. This is all a huge amount of work. " https://medium.com/@tridge60/rsync-and-outrage-d9849599e5a0 https://medium.com/@tridge60/rsync-and-outrage-d9849599e5a0
- rendaw 4mo ago
  Is using calloc for everything fixing a security issue or hardening it?
  - ekidd 4mo ago
    Calloc is generally hardening, because it zeros out any stale memory contents left over from previous uses of the memory. You can avoid this overhead if you use a language that forbids reading from uninitialized memory, but C is not that language.
    - kelnos 4mo ago
      Uninitialized memory is not a problem (the OS is never going to give a program memory that has data in it from another program). The problem is memory that you allocated in the past, have freed, but hasn't been returned to the OS[0]. It might have key material or other sensitive data in it[1]. Or it might just have random garbage in it that could be misinterpreted by the code that's about to use it, if it hasn't been initialized to a known state. For some uses, you do genuinely need (specifically) zeroed-out memory before you start to use it, and that's where calloc() is truly useful. But that need not have anything to do with security. [0] The allocator will often hold onto memory that has been freed in order to quickly service future requests for new allocations, without needing a context switch into kernel space. [1] Granted, the correct way to handle that is to zero it out before freeing it, in a way that the compiler won't optimize out.
      - ekidd 4mo ago
        > The problem is memory that you allocated in the past, have freed, but hasn't been returned to the OS[0]. There are at least two different ways in which memory might be semantically "uninitialized": 1. The memory was provided by the OS. On modern desktop and mobile OSes, this memory will normally be zeroed automatically. 2. The memory was provided by the language's allocator. This may contain a mix of data used by previous allocations and memory that has never been touched (perhaps because previous allocations reserved it as end-of-array "capacity" that never got used). From the perspective of a language like Rust, this memory is considered uninitialized, and safe code should never be able to read it without first setting it. In ancient C code, it makes a fair bit of sense to preemptively calloc everything. Or better, to wrap the allocator with one that zeroes on free. Though even there, you need to be careful not to expose recycled heap block headers in the middle of newly allocated objects. My opinion for the last 30+ years has been that C is unfit for purpose, and that using it almost inevitably introduces large numbers of dire security holes. But until the last 10-15 years, there hasn't been any seriously viable alternatives.
gravypod 4mo ago
> Also the amount of commits is suspicious. In the last two months, rsync had about as much commits as in the last two years before that. I wonder if the data looks worse or better when not doing per-10commit and instead do per-commit.
echelon 4mo ago
Seems like someone could use Claude to port rsync to Rust and the whole enterprise would be safer from things like this. Start with unsafe then gradually convert into idiomatic Rust.
- yubblegum 4mo ago
  Your let's redo this in Rust made me wonder if generative AI will also be susceptible to software fads. One LLM writes a few blog posts extoling a new framework/lanaguge. Other agentics read these and get 'influenced'. Then they start clamoring for 'lets redo this in X!'. Can't wait to see it. /g
  - whattheheckheck 4mo ago
    We will need rigorous agnostic statistical experiments to know what stuff is better
  - globalnode 4mo ago
    its bad enough when humans do it
  - bryanrasmussen 4mo ago
    Prompt: automate writing commits to increase safety in these software projects so that my profile increases and I can snag a high-paying Rust job. LLM: this commit changes whole codebase to Rust!
  - kajaktum 4mo ago
    You can get 80% there with rust which is what is impressive. Then you have a reference implementation that you can always check against. If a Rust library have 0 unsafe, i dont care if it is written by a dog, it still have 0 UB.
    - klabb3 4mo ago
      UB is especially bad but also not as big as all other concerns combined. Two of the most reliable software ever to exist, curl and SQLite, are C/C++. There are also cases in system programming, drivers etc where the unsafe is necessary and then your code is only as good as the boundary, and lots of bugs can seep in. Another issue with Rust is ecosystem - the dependency trees required to do fairly basic things are often deep and vast, meaning other risks. That said if something like rsync was written today, I still think Rust may be a better choice. Mainly because a 95 percentile skilled Rust programmer is less dangerous than for C. The people that are skilled enough to be trusted with C are few and diminishing every year.
lokar 4mo ago
I would expect a 10x change rate, even carried out by clones of the existing maintainers to result in more bugs.
whateveracct 4mo ago
mythical man month only gets more prescient as time passes

scottlamb 4mo ago

> This is a good example of what slips through LLM attention. It forces all allocations to be calloc as if it is a strict upgrade. I wouldn't assume Claude made that decision; it's not as if that was some incidental thing that it snuck into a large commit. The commit message starts with "zero all new memory from allocations", and that's exactly what the commit does. What do you imagine the prompt was? It seems totally plausible to me that a human initially thought this was an improvement, then rethought after discovering the RSS regression. And it's not a law of nature anyway that this change has to increase RSS; calloc could special-case the case in which memory was freshly returned from the OS, knowing fresh memory mappings are zeroed anyway. I blame AI for these regressions mostly in the sense that it caused a flurry of vulnerability reports. Those led to a flurry of quick fixes. Sometimes quick fixes cause other problems.

delusional 4mo ago
You don't really have to guess. The guy told us the AI didn't suggest this specific change: > The change to zero memory was my idea and my change. It was a reaction to a security report I got which caused use of an element past the end of an array. By zeroing the allocation I could ensure that misuse of that memory if a similar bug came up in the future could only cause a null ptr deref, which is better than the chance of a valid pointer. It got a claude co-authored tag on it as I got it to do some tidy ups of a series of commits, and that is just what it does when it makes any modification. It doesn't mean the change was written by claude. It was written by me. https://github.com/RsyncProject/rsync/issues/959#issuecomment-4625643726 https://github.com/RsyncProject/rsync/issues/959#issuecommen...
- jagged-chisel 4mo ago
  > … By zeroing the allocation … How does that prevent reading past the end of the buffer? Or change how bytes outside the buffer are used? Are these arrays of pointers so that the “null ptr deref” comment makes sense? Or am I the bozo and don’t know what’s happening here?
  - kccqzy 4mo ago

tom_ 4mo ago

AI multiplied by Linux overcommit. What times we live in! (My own view: 10.8 GB is nothing these days. Your sprintf buffers are probably larger than that. (And if they aren't: they should be. That, or you should start using snprintf...))

baq 4mo ago
sprintf() should be a longer way to write abort(), change my mind
- bruce343434 4mo ago
  I'll change your mind: If you pass NULL as the destination pointer, it doesn't write any string. If you combine this with %n at the end of the format string, you can get the exact length that the output string would be. Then you allocate that, then you print again, into the actual destination buffer this time.
  - baq 4mo ago
    If anything I just got entrenched in my opinion. The second best option, maybe, would be to not accept any destination pointers and have the default and only possible behavior just like what you describe.
    - bruce343434 4mo ago
      I agree. I tend to use the gnu "asprintf" which simply returns a properly allocated char buffer with the formatted string in it. And on platforms that don't feature asprintf (windows) you can build your own using sprintf!

CaliforniaKarl 4mo ago

> Written with claude. No. The reversion commit references https://github.com/RsyncProject/rsync/issues/959 https://github.com/RsyncProject/rsync/issues/959. In that GitHub issue is this comment: > The change to zero memory was my idea and my change. It was a reaction to a security report I got which caused use of an element past the end of an array. By zeroing the allocation I could ensure that misuse of that memory if a similar bug came up in the future could only cause a null ptr deref, which is better than the chance of a valid pointer. > It got a claude co-authored tag on it as I got it to do some tidy ups of a series of commits, and that is just what it does when it makes any modification. It doesn't mean the change was written by claude. It was written by me.

alfiedotwtf 4mo ago

AI is fine, and in fact fun to use... committing AI written code without understanding Every. Single. Line. Of. Changes is on the committer. You can't LGFM for vibe code ffs

KaiShips 4mo ago

[flagged]